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

Force the use of secure TLS config #873

Merged
merged 1 commit into from
Nov 18, 2021
Merged

Conversation

enj
Copy link
Contributor

@enj enj commented Oct 19, 2021

This change updates the TLS config used by all pinniped components.
There are no configuration knobs associated with this change. Thus
this change tightens our static defaults.

There are four TLS config levels:

  1. Secure (TLS 1.3 only)
  2. Default (TLS 1.2+ best ciphers that are well supported)
  3. Default LDAP (TLS 1.2+ with less good ciphers)
  4. Legacy (currently unused, TLS 1.2+ with all non-broken ciphers)

Highlights per component:

  1. pinniped CLI
    • uses "secure" config against KAS
    • uses "default" for all other connections
  2. concierge
    • uses "secure" config as an aggregated API server
    • uses "default" config as a impersonation proxy API server
    • uses "secure" config against KAS
    • uses "default" config for JWT authenticater (mostly, see code)
    • no changes to webhook authenticater (see code)
  3. supervisor
    • uses "default" config as a server
    • uses "secure" config against KAS
    • uses "default" config against OIDC IDPs
    • uses "default LDAP" config against LDAP IDPs

Signed-off-by: Monis Khan mok@vmware.com

Release note:

TLS 1.2+ with a modern set of TLS ciphers is now required for all connections coming into or going out of all pinniped components.

@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #873 (53b8b72) into main (c570f08) will decrease coverage by 0.56%.
The diff coverage is 54.39%.

❗ Current head 53b8b72 differs from pull request most recent head cd686ff. Consider uploading reports for the commit cd686ff to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #873      +/-   ##
==========================================
- Coverage   79.04%   78.48%   -0.57%     
==========================================
  Files         129      132       +3     
  Lines        9245     9471     +226     
==========================================
+ Hits         7308     7433     +125     
- Misses       1706     1784      +78     
- Partials      231      254      +23     
Impacted Files Coverage Δ
internal/concierge/server/server.go 27.04% <0.00%> (-0.46%) ⬇️
internal/net/phttp/debug.go 7.40% <7.40%> (ø)
internal/concierge/impersonator/impersonator.go 82.93% <46.87%> (-4.14%) ⬇️
cmd/pinniped/cmd/kubeconfig.go 84.18% <50.00%> (+1.02%) ⬆️
internal/kubeclient/kubeclient.go 56.86% <50.00%> (-12.19%) ⬇️
...ler/authenticator/jwtcachefiller/jwtcachefiller.go 86.27% <65.21%> (-6.50%) ⬇️
internal/crypto/ptls/ptls.go 68.91% <68.91%> (ø)
cmd/pinniped/cmd/login_oidc.go 91.62% <100.00%> (-0.36%) ⬇️
...enticator/webhookcachefiller/webhookcachefiller.go 96.15% <100.00%> (ø)
...l/controller/kubecertagent/pod_command_executor.go 88.88% <100.00%> (+88.88%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c570f08...cd686ff. Read the comment docs.

@enj enj force-pushed the enj/i/strict_tls branch 2 times, most recently from 5a24a5c to 8e667f7 Compare October 20, 2021 12:00
@enj enj changed the title [WIP] Force the use of secure TLS config Force the use of secure TLS config Nov 16, 2021
This change updates the TLS config used by all pinniped components.
There are no configuration knobs associated with this change.  Thus
this change tightens our static defaults.

There are four TLS config levels:

1. Secure (TLS 1.3 only)
2. Default (TLS 1.2+ best ciphers that are well supported)
3. Default LDAP (TLS 1.2+ with less good ciphers)
4. Legacy (currently unused, TLS 1.2+ with all non-broken ciphers)

Highlights per component:

1. pinniped CLI
   - uses "secure" config against KAS
   - uses "default" for all other connections
2. concierge
   - uses "secure" config as an aggregated API server
   - uses "default" config as a impersonation proxy API server
   - uses "secure" config against KAS
   - uses "default" config for JWT authenticater (mostly, see code)
   - no changes to webhook authenticater (see code)
3. supervisor
   - uses "default" config as a server
   - uses "secure" config against KAS
   - uses "default" config against OIDC IDPs
   - uses "default LDAP" config against LDAP IDPs

Signed-off-by: Monis Khan <mok@vmware.com>
@enj
Copy link
Contributor Author

enj commented Nov 17, 2021

Error message when compiled with an older version of Go:

# go.pinniped.dev/internal/crypto/ptls
internal/crypto/ptls/old.go:11:2: "Pinniped's TLS configuration makes assumptions about how the Go standard library implementation of TLS works.\nIt particular, we rely on the server controlling cipher suite selection.  For these assumptions to hold, Pinniped\nmust be compiled with Go 1.17+.  If you are seeing this error message, your attempt to compile Pinniped with an\nolder Go compiler was explicitly failed to prevent an unsafe configuration." evaluated but not used
note: module requires Go 1.17

@enj enj merged commit 6a68c65 into vmware-tanzu:main Nov 18, 2021
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.

None yet

3 participants