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

Work around Security Framework failing to import PKCS12 from OpenSSL 3 #693

Closed
wants to merge 2 commits into from

Conversation

kazk
Copy link
Member

@kazk kazk commented Nov 5, 2021

Should be a workaround for #691.

OpenSSL 3 made the following breaking changes for PKCS#12:

PKCS#12 API updates

The default algorithms for pkcs12 creation with the PKCS12_create() function were changed to more modern PBKDF2 and AES based algorithms. The default MAC iteration count was changed to PKCS12_DEFAULT_ITER to make it equal with the password-based encryption iteration count. The default digest algorithm for the MAC computation was changed to SHA-256. The pkcs12 application now supports -legacy option that restores the previous default algorithms to support interoperability with legacy systems.
https://www.openssl.org/docs/man3.0/man7/migration_guide.html#PKCS-12-API-updates

Security Framework used by native_tls on macOS and iOS fails to import PKCS#12 with modern cyphers.

Work around this by using the legacy algorithms on macOS.

I don't know if Windows is affected, we can just change the #[cfg] if it is.


@danni-m

Can you try this branch?

kube = { git = "https://github.com/kazk/kube-rs", branch = "fix-macos-openssl3", features = ["derive", "native-tls"] }

@danni-m
Copy link

danni-m commented Nov 7, 2021

@kazk

 dannimoiseyev@danni-mxbp  ~/code/test_kube   main ±  cargo run  
    Finished dev [unoptimized + debuginfo] target(s) in 0.23s
     Running `target/debug/test_kube`
Error: OpensslError: error:0308010C:digital envelope routines:inner_evp_generic_fetch:unsupported:crypto/evp/evp_fetch.c:346:Global default library context, Algorithm (RC2-40-CBC : 0), Properties (), error:11800067:PKCS12 routines:PKCS12_item_i2d_encrypt_ex:encrypt error:crypto/pkcs12/p12_decr.c:191:, error:11800067:PKCS12 routines:PKCS12_pack_p7encdata_ex:encrypt error:crypto/pkcs12/p12_add.c:127:

Caused by:
    error:0308010C:digital envelope routines:inner_evp_generic_fetch:unsupported:crypto/evp/evp_fetch.c:346:Global default library context, Algorithm (RC2-40-CBC : 0), Properties (), error:11800067:PKCS12 routines:PKCS12_item_i2d_encrypt_ex:encrypt error:crypto/pkcs12/p12_decr.c:191:, error:11800067:PKCS12 routines:PKCS12_pack_p7encdata_ex:encrypt error:crypto/pkcs12/p12_add.c:127:

@kazk
Copy link
Member Author

kazk commented Nov 7, 2021

Ah, looks like OpenSSL 3 can't load RC2-40-CBC at the moment. We're blocked by sfackler/rust-openssl#1553 which is in turn blocked by openssl/openssl#16970 .

Maybe we can do Nid::PBE_WITHSHA1AND3_KEY_TRIPLEDES_CBC for cert_algorithm (OPENSSL_NO_RC2). Can you try again?

@nightkr
Copy link
Member

nightkr commented Nov 8, 2021

Is there anything blocking us from promoting OpenSSL everywhere? If Security.Framework is this broken then I'd rather steer people away from it than default to bad crypto.

Besides, at least for controllers this'd be more representative of the actual target environment anyway.

@nightkr
Copy link
Member

nightkr commented Nov 8, 2021

Okay.. after reading through this again then it looks like it's not a huge issue since it's only for adapting the cert in-memory at runtime, since they don't support direct loading from PEM?

If so then I'd rather apply the workaround globally but slap a warning on the function that the PKCS12 bundles it generates shouldn't be transmitted anywhere.

@kazk
Copy link
Member Author

kazk commented Nov 8, 2021

Yeah, PKCS#12 is necessary because native-tls doesn't have PKCS#8 support at the moment. We're creating PKCS#12 to immediately unpack 🤦‍♂️

I've picked up the old PR to add PKCS#8 support, and working with them to get that merged (sfackler/rust-native-tls#209).

@kazk
Copy link
Member Author

kazk commented Nov 8, 2021

Is there anything blocking us from promoting OpenSSL everywhere?

We can add another TLS feature flag openssl-tls (for https://crates.io/crates/hyper-openssl), but I don't think we can drop native-tls. It's a pain to build OpenSSL in some platforms, and there are users who need native-tls. The best is rustls everywhere, but that's not ready.

If so then I'd rather apply the workaround globally

Yeah, that's fine.

slap a warning on the function that the PKCS12 bundles it generates shouldn't be transmitted anywhere.

We can add it, but I don't see the point of the warning. PKCS#12 with legacy algorithm has been the default and will continue to be more common for a long time. It's a legacy format anyway.

pkcs12_from_pem is a private function and the generated package is immediately used:

https://github.com/kube-rs/kube-rs/blob/2b5e4ad788366125448ad40eadaf68cf9ceeaf31/kube-client/src/client/tls.rs#L17-L19

It's also obviously insecure because " " is the password.

Hopefully PKCS#8 support gets merged soon, and we don't need to workaround.

@nightkr
Copy link
Member

nightkr commented Nov 8, 2021

It's a pain to build OpenSSL in some platforms, and there are users who need native-tls.

Doesn't rustc typically depend on the native C toolchain's linker anyway, so I'm not sure we're really saving that much trouble here? Although this may have changed since the last time I looked at it, I remember that there were some experiments around migrating to bunding LLVM's lld instead.

The best is rustls everywhere, but that's not ready.

100%

We can add it, but I don't see the point of the warning. PKCS#12 with legacy algorithm has been the default and will continue to be more common for a long time. It's a legacy format anyway.

There's a difference between "this use of this function is obviously insecure" and "this function is inherently insecure, no matter how you use it".

@kazk
Copy link
Member Author

kazk commented Nov 8, 2021

Doesn't rustc typically depend on the native C toolchain's linker anyway, so I'm not sure we're really saving that much trouble here?

native-tls is not saving any trouble at the moment because openssl is required anyway for PKCS#12 :D

I don't know how frustrating things are on macOS or Windows or anything else, but setting up OpenSSL on Windows for CI was a huge pain. I've read devs on Windows having trouble with OpenSSL multiple times, too. The vendored feature should help, but it takes a while to compile from source.

@kazk
Copy link
Member Author

kazk commented Nov 8, 2021

Hmm, maybe the only reason we have native-tls is historical? Maybe it's because reqwest didn't have openssl-tls feature? It's pretty awkward to use native-tls if openssl is required.

native-tls is a nice to have option once PKCS#8 is supported, though.

@kazk
Copy link
Member Author

kazk commented Nov 17, 2021

Until PKCS8 is supported by native-tls, the new openssl-tls feature can be used.

@kazk kazk closed this Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants