-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add HTTP engine config for min TLS version #844
Conversation
@@ -69,6 +71,8 @@ private fun OkHttpEngineConfig.buildClient(): OkHttpClient { | |||
followRedirects(false) | |||
followSslRedirects(false) | |||
|
|||
connectionSpecs(listOf(minTlsConnectionSpec(config.minTlsVersion), ConnectionSpec.CLEARTEXT)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Why do we need to add CLEARTEXT
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're replacing the default list of all acceptable connections, not just TLS ones. If we don't add CLEARTEXT
, then HTTP connections will be impossible.
@@ -72,6 +73,8 @@ public open class HttpClientEngineConfig constructor(builder: Builder) { | |||
*/ | |||
public val proxySelector: ProxySelector = builder.proxySelector | |||
|
|||
public val minTlsVersion: TlsVersion = builder.minTlsVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Should we be forward looking and define something like TlsContext
to group TLS settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts here--
I would prefer it from an organization perspective, but if we go that route I think we'd want to reconsider grouping all of the connection based things (timeouts, max conns) as well.
I think that would be liable to complicate operation config override for these settings (which AFAIK we wanted to revisit being able to do independently of setting the HTTP client). e.g. Instead of patching one field in the config, I now have to construct a new container, merge in what i want to stay the same, and apply overrides.
Not taking a hard stance here, more just trying to play out the different paths forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TlsContext
added and I've also moved the existing alpn
setting into it alongside the new TLS minimum version.
Concerns about complications for overriding client config are most definitely valid but won't be solved in this PR. We'll tackle that in the near future with a larger overhaul.
/** | ||
* TLS version 1 | ||
*/ | ||
Tls1_0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: For other enums like AlpnId
we use screaming snake case, TLS1_0
, TLS1_1
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears we're inconsistent between SCREAMING_CASE (e.g., AlpnId
, PresigningLocation
, TimestampFormat
, etc.) and PascalCase (e.g., ErrorType
, Phase
, OsFamily
, etc.). Interestingly, even Kotlin's own style guide allows either "depending on usage". I can switch this to SCREAMING_CASE since that matches Kotlin's documented examples but I wonder if we need to be more consistent throughout our project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we probably should be. Probably even worth a lint 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agreed on lint rules for whatever we decide. The default ktlint ruleset already verifies enum names although it allows both styles permitted by JB's Kotlin guidelines. You may notice that I suppressed this rule on the enum since I wrote it in PascalCase but needed _
as a digit separator. Maybe I should've gone for Tls1Dot1
(/s).
* Specifies the minimum acceptable version of TLS to use when connecting to service endpoints. | ||
*/ | ||
public val MinTlsVersion: EnvironmentSetting<TlsVersion> = | ||
enumEnvSetting<TlsVersion>("SDK_MIN_TLS", "sdk.minTls").orElse(TlsVersion.Tls1_2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concern: I think we really ought to use a namespace we own like aws.smithy
or aws.sdk
(I was late to other PR but left same comment for retries).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to defer (re)naming this setting until standards exist for cross-SDK envvar/sysprop config naming conventions.
…gine-specific TlsContext from CRT engine config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
*/ | ||
package aws.smithy.kotlin.runtime.http.test | ||
|
||
// TODO Finish once we have HTTP engine support for client certificates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a note to the TLS configuration issue to finish this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added note to #820.
*/ | ||
@InternalApi | ||
public fun <T> EnvironmentSetting<T>.resolve(platform: PlatformEnvironProvider): T? { | ||
public fun <T> EnvironmentSetting<T>.resolve(platform: PlatformEnvironProvider = PlatformProvider.System): T? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change here, in my LogMode PR I passed PlatformProvider.System
but it's better to add a default
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Issue #
Closes #661
Description of changes
This change adds a new
TlsContext
subgroup in generic HTTP engine config, to which I've added a newminVersion
field for setting the minimum allowable TLS version. I've also movedalpn
into the TLS context since it seems related. Finally, to prevent confusion or namespacing issues, I've removedTlsContext
from the CRT-specific engine config.Testing
Unfortunately, testing this in an automated way is not yet possible. I've started some test harness implementation but won't be able to complete it until HTTP engines also support certification configuration, which is necessary to allow self-signed certificates for connecting to private HTTPS servers.
Breaking changes
alpn
fromHttpClientEngineConfig
to newTlsContext
classTlsContext
onCrtHttpEngineConfig
See the BREAKING: Streamlined TLS configuration post for more details.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.