- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 364
Change the default TLS to OpenSSL #863
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
Conversation
      
        
              This comment was marked as spam.
        
        
      
    
  This comment was marked as spam.
bc5f2e5    to
    3726230      
    Compare
  
    | - name: install openssl | ||
| if: matrix.os == 'windows-latest' | ||
| run: | | ||
| $ErrorActionPreference = "Stop" | 
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.
https://github.com/kube-rs/kube-rs/runs/5766982332?check_suite_focus=true failed to install OpenSSL, but this step didn't fail. Setting this should stop it.
It's usually prepended automatically, but we have fail-fast: false.
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference
| Yeah, I agree with this as well. I was slightly worried it was going to impact the musl builds somehow (as it bundles its own openssl, possibly avoiding the native-tls magic), but  | 
| Cool, I'll rebase and merge this. What do you think of removing  | 
`native-tls` feature exists because `kube` used to depend on `reqwest`. The feature doesn't make sense for us because all targets still depend on `openssl` anyway. This is because `native-tls` requires PKCS#12 to create `Identity` for authentication with client certificates, and `openssl` is the only trusted option to generate them. A feature to create `Identity` from PKCS#8 was added a few days ago, but `openssl` is still necessary because we need to support more private key formats. `native-tls` feature is currently broken on macOS because Security Framework cannot import PKCS#12 generated by OpenSSL v3 (kube-rs#691). https://openradar.appspot.com/FB8988319 Signed-off-by: kazk <[email protected]>
32c3bbe    to
    0a97484      
    Compare
  
    | I just noticed all the tests requiring a cluster are skipped except for e2e. We should be able to run them in  | 
| 
 I think that also makes sense. However, for sanity, maybe best to give it one version between changing default and removing. | 
| 
 coverage job also runs integration tests | 
| 
 Good idea. 
 👍 
 macOS doesn't have Docker installed because of some licensing issue. On Windows, GitHub Action to set up k3d doesn't even run because of some kind of path issue.  | 
| I think this might need a follow-up: the default from the facade-crate is still  | 
Motivation
native-tlsfeature exists becausekubeused to depend onreqwest.The feature doesn't make sense for us because all targets still depend on
opensslanyway. This is becausenative-tlsrequires PKCS#12 to createIdentityfor authentication with client certificates, andopensslis the only trusted option to generate them.A feature to create
Identityfrom PKCS#8 was added a few days ago, butopensslis still necessary because we need to support more private key formats.native-tlsfeature is currently broken on macOS because Security Framework cannot import PKCS#12 generated by OpenSSL v3 (#691). I'm sure more users will run into this, especially the new ones, so it's not a good default.Solution
Make OpenSSL the default because we can't make Rustls due to #153. I don't think there are any downsides?
We can also remove
native-tlsfeature.opensslwill continue to be required because I don't think the different key formats we need will be supported anytime soon.