Skip to content
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

Introduce new build tags to optionally override some min and max TLS settings #2162

Merged
merged 2 commits into from
Dec 20, 2024

Conversation

cfryanr
Copy link
Member

@cfryanr cfryanr commented Dec 19, 2024

This PR introduces two new build tags which can be used to override some of the default min TLS and max TLS settings in Pinniped at build time.

  1. When building the server code using docker build -f hack/Dockerfile_fips, you can now add the optional argument --build-arg ADDITIONAL_BUILD_TAGS=fips_enable_tls13_max_for_default_profile. This will cause the Concierge and Supervisor servers to allow both TLS v1.2 and TLS v1.3 for all incoming and outgoing connections. By default, it will only use TLS v1.2 for these purposes because the current version of boringcrypto used by go1.23.x does not support TLS v1.3. However, if you are using a custom Go compiler and your version of boringcrypto does support TLS v1.3, then you could choose to enable this new option.

  2. When building the server code using docker build . (with the default Dockerfile at the project's root directory), you can now add an optional argument --build-arg ADDITIONAL_BUILD_TAGS=nonfips_enable_tls12_min_for_secure_profile. This will cause the Concierge and Supervisor servers to allow both TLS v1.2 and TLS v1.3 when serving their aggregated APIs to the Kubernetes API server, and when creating outgoing connections to the Kubernetes API server. By default, it will only allow TLS v1.3 for these purposes, because the Kubernetes API server always allows TLS v1.3.

  3. When building the CLI in regular non-FIPS mode, you can optionally use the build tag go build -tag nonfips_enable_tls12_min_for_secure_profile ./cmd/pinniped to cause the CLI to allow both TLS v1.2 and TLS v1.3 when creating outgoing connections to the Kubernetes API server. By default, it will only allow TLS v1.3 for these purposes, because the Kubernetes API server always allows TLS v1.3.

Note that we do not yet test building the CLI in "FIPS mode" (using the fips_strict build tag). However, if you did do that, you could theoretically use the same build tag mentioned in item number 1 above to achieve the same result for all the CLI's outgoing connections to allow both TLS v1.2 and v1.3. However, this is not tested today.

Release note:

TBD

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for pinniped-dev canceled.

Name Link
🔨 Latest commit ef4b0c9
🔍 Latest deploy log https://app.netlify.com/sites/pinniped-dev/deploys/6765b7733f5385000894299f


import "crypto/tls"

const SecureProfileMinTLSVersionForNonFIPS = tls.VersionTLS12

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name does not match the value defined in internal/crypto/ptls/profiles_fips_strict.go L76,
config.MaxVersion = DefaultProfileMaxTLSVersionForFIPS

Copy link
Member Author

@cfryanr cfryanr Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @luwangVMW,

There are two new build tags:

  1. nonfips_enable_tls12_min_for_secure_profile is only for non-FIPS builds. By default, there is a global const called SecureProfileMinTLSVersionForNonFIPS set to 1.3. When the build tag is used during compilation, it changes the value of that const to be 1.2. This variable is only used in internal/crypto/ptls/profiles.go which is the file used in non-FIPS builds.
  2. fips_enable_tls13_max_for_default_profile is only for FIPS builds. By default, there is a global const called DefaultProfileMaxTLSVersionForFIPS set to 1.2. When the build tag is used during compilation, it changes the value of that const to be 1.3. This variable is only used in internal/crypto/ptls/profiles_fips_strict.go which is the file used in FIPS builds.

Side note: In the future, I expect that the official golang compiler will use a version of boringcrypto which supports TLS 1.3. The golang maintainers previously estimated that this could happen in Go 1.24 in February 2025. If that happens, then we may be able to enable TLS 1.3 by default when compiled in FIPS mode, so we may not need the setting fips_enable_tls13_max_for_default_profile described above at all because it would be the same as our new default.

@joshuatcasey
Copy link
Member

I wonder if we could clarify the build tag names?

default_profile_max_tls_for_fips_13 -> fips_enable_tls13_max_for_default_profile?
secure_profile_min_tls_for_nonfips_12 -> nonfips_enable_tls12_min_for_secure_profile?

@cfryanr cfryanr force-pushed the build_tags_for_tls_versions branch from 2768d97 to ef4b0c9 Compare December 20, 2024 18:29
@cfryanr
Copy link
Member Author

cfryanr commented Dec 20, 2024

I wonder if we could clarify the build tag names?

default_profile_max_tls_for_fips_13 -> fips_enable_tls13_max_for_default_profile? secure_profile_min_tls_for_nonfips_12 -> nonfips_enable_tls12_min_for_secure_profile?

I made those updates in the code as well as in my previous comments above, to keep the code and my comments consistent.

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 31.94%. Comparing base (acbe9ce) to head (ef4b0c9).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/crypto/ptls/profiles.go 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2162      +/-   ##
==========================================
- Coverage   31.94%   31.94%   -0.01%     
==========================================
  Files         379      379              
  Lines       62093    62098       +5     
==========================================
+ Hits        19838    19839       +1     
- Misses      41725    41728       +3     
- Partials      530      531       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joshuatcasey joshuatcasey merged commit b4365c1 into main Dec 20, 2024
39 checks passed
@joshuatcasey joshuatcasey deleted the build_tags_for_tls_versions branch December 20, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants