Conversation
…registry_client.rs for cargo build
cargo fmt
geofft
left a comment
There was a problem hiding this comment.
If you've already talked/thought about this please disregard, but, I'd be a little hesitant to turn on OCSP checking by default. The security benefit is minimal (assuming that it's in fail-open mode when the OCSP responder cannot be reached) and the potential for inexplicable hangs or connection failures seems high.
| a PEM-encoded certificate bundle (e.g., `certs.pem`, `ca-bundle.crt`), or set | ||
| [`SSL_CERT_DIR`](../../reference/environment.md#ssl_cert_dir) to one or more directories containing | ||
| PEM-encoded certificate files. Multiple entries are supported, separated using a platform-specific |
There was a problem hiding this comment.
Do you load every file in the directory, or do you need the OpenSSL-style abcd1234.0 hash symlinks to be present (man openssl-rehash)? Do you read every file regardless of extension in that directory?
(I see the mention of "dropping support for OpenSSL hash-named certificate files" in your PR description but I don't totally follow what that means about the current state of things.)
There was a problem hiding this comment.
#17543 reimplemented the logic from rustls-native-certs and accidentally dropped support for the OpenSSL-style files.
Here, we retain use of rustls-native-certs for loading and thus follow the semantics defined there. Previously, they gated reads to OpenSSL-style files and other known certificate extensions, but as of the latest version, they read every file.
I do not know if we have control over this. I will look into it though. There has not been prior discussion, this item in the description is just from me exploring the possible implications of the dependency change. |
|
We chatted briefly in Discord — I think OCSP is not something we can control but should be fairly limited. |
Co-authored-by: Zanie Blue <contact@zanie.dev>
34dc0bc to
a5ef05c
Compare
| sudo dnf install gcc | ||
| ``` | ||
|
|
||
| On Windows, [NASM](https://www.nasm.us/) is required for building the TLS backend (`aws-lc-sys`). If |
There was a problem hiding this comment.
Should we expose the cargo feature for user and distributors that don't want to compile NASM?
There was a problem hiding this comment.
Hm I thought the feature was already enabled, let me double check...
There was a problem hiding this comment.
The following user-facing changes are included here: - `aws-lc` is used instead of `ring` for a cryptography backend - Expands our certificate signature algorithm support to include ECDSA_P256_SHA512, ECDSA_P384_SHA512, ECDSA_P521_SHA256, ECDSA_P521_SHA384, and ECDSA_P521_SHA512 - `--native-tls` is deprecated in favor of a new `--system-certs` flag, avoiding confusion with the TLS implementation used (we use `rustls` not `native-tls`, see prior confusion at #11595) - NASM is a new build requirement on Windows, it is required by `aws-lc` on x86-64 and i386 - `rustls-platform-verifier` is used instead of `rustls-native-certs` for system certificate verification - On macOS, certificate validation is now delegated to `Security.framework` (`SecTrust`). Performance when using `--system-certs` is improved by avoiding exporting and parsing all the certificates from the keychain at startup. - On Windows, certificate validation is now delegated to `CertGetCertificateChain` and `CertVerifyCertificateChainPolicy` - On Linux, certificate validation should be approximately unchanged - Some previously failing chains may succeed, and some previously accepted chains may fail; generally, this should result in behavior closer matching browsers and other native applications - macOS and Windows may now perform live OCSP fetches for early revocation, which could add latency to some requests - Empty `SSL_CERT_FILE` values are ignored (for consistency with `SSL_CERT_DIR`) The following internal changes are included here: - Certificate loading has been refactored to use a newtype with helper methods - The certificate tests have been rewritten - We use `webpki-root-certs` instead of `webpki-roots`, see #17543 (comment) - We request `identity` encoding for range requests, see astral-sh/async_http_range_reader#3 (comment) - Various dependencies (including forks) updates to versions which use reqwest 0.13+ This is a replacement of #17543 with an updated description. See that pull request for prior discussion. I've made the following changes from the initial approach there: - Previously, the `native-tls` TLS implementation was added which included an OpenSSL build. We don't currently use the `native-tls` implementation, but the `--native-tls` flag there was erroneously updated to enable it. - Previously, there was a `--tls-backend` flag to toggle between `native-tls` and `rustls`. Since we currently always use `rustls`, this is deferred to future work (if we need it at all). - Previously, there were unintentional breaking changes to `SSL_CERT_FILE` and `SSL_CERT_DIR` handling, including merging with the base certificates instead of replacing them, dropping support for OpenSSL hash-named certificate files, skipping deduplication of certificates. Here, we retain use of `rustls-native-certs` for loading certificates from the system as it handles these edge cases. Closes #17427 --------- Co-authored-by: salmonsd <22984014+salmonsd@users.noreply.github.com>
The following user-facing changes are included here:
aws-lcis used instead ofringfor a cryptography backend--native-tlsis deprecated in favor of a new--system-certsflag, avoiding confusion with the TLS implementation used (we userustlsnotnative-tls, see prior confusion at Consider using the system SSL library, i.e., OpenSSL instead ofrusttls/ring#11595)aws-lcon x86-64 and i386rustls-platform-verifieris used instead ofrustls-native-certsfor system certificate verificationSecurity.framework(SecTrust). Performance when using--system-certsis improved by avoiding exporting and parsing all the certificates from the keychain at startup.CertGetCertificateChainandCertVerifyCertificateChainPolicySSL_CERT_FILEvalues are ignored (for consistency withSSL_CERT_DIR)The following internal changes are included here:
webpki-root-certsinstead ofwebpki-roots, see Update Reqwest to0.13.1#17543 (comment)identityencoding for range requests, see build: update Reqwest to0.13.1async_http_range_reader#3 (comment)This is a replacement of #17543 with an updated description. See that pull request for prior discussion. I've made the following changes from the initial approach there:
native-tlsTLS implementation was added which included an OpenSSL build. We don't currently use thenative-tlsimplementation, but the--native-tlsflag there was erroneously updated to enable it.--tls-backendflag to toggle betweennative-tlsandrustls. Since we currently always userustls, this is deferred to future work (if we need it at all).SSL_CERT_FILEandSSL_CERT_DIRhandling, including merging with the base certificates instead of replacing them, dropping support for OpenSSL hash-named certificate files, skipping deduplication of certificates. Here, we retain use ofrustls-native-certsfor loading certificates from the system as it handles these edge cases.Closes #17427