Closed alilleybrinker closed 3 months ago
alilleybrinker
commented
5 months ago Ended up opting to rebase down to one commit, and also fixed the table reformatting so the table is no longer reformatted at all, just has a row added for the new flag. If you'd still like that in its own commit separately, let me know.
coveralls
commented
4 months ago This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| src/steps/hook.rs | 0 | 1 | 0.0% | ||
| src/steps/mod.rs | 0 | 1 | 0.0% | ||
| src/steps/replace.rs | 0 | 1 | 0.0% | ||
| src/ops/cargo.rs | 0 | 2 | 0.0% | ||
| src/steps/publish.rs | 0 | 2 | 0.0% | ||
| src/config.rs | 1 | 5 | 20.0% | ||
| src/steps/release.rs | 0 | 7 | 0.0% | ||
| src/ops/index.rs | 0 | 13 | 0.0% | ||
| <!-- | Total: | 1 | 32 | 3.13% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| src/ops/index.rs | 1 | 0.0% | ||
| src/ops/replace.rs | 1 | 0.0% | ||
| src/config.rs | 6 | 26.69% | ||
| <!-- | Total: | 8 | --> |
| Totals | |
|---|---|
| Change from base Build 8940883685: | -0.07% |
| Covered Lines: | 218 |
| Relevant Lines: | 2471 |
coveralls
commented
3 months ago This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| src/steps/hook.rs | 0 | 1 | 0.0% | ||
| src/steps/mod.rs | 0 | 1 | 0.0% | ||
| src/steps/replace.rs | 0 | 1 | 0.0% | ||
| src/ops/cargo.rs | 0 | 2 | 0.0% | ||
| src/steps/publish.rs | 0 | 2 | 0.0% | ||
| src/config.rs | 1 | 6 | 16.67% | ||
| src/steps/release.rs | 0 | 7 | 0.0% | ||
| src/ops/index.rs | 0 | 13 | 0.0% | ||
| <!-- | Total: | 1 | 33 | 3.03% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| src/ops/index.rs | 1 | 0.0% | ||
| src/config.rs | 6 | 26.69% | ||
| <!-- | Total: | 7 | --> |
| Totals | |
|---|---|
| Change from base Build 9502945012: | -0.04% |
| Covered Lines: | 218 |
| Relevant Lines: | 2455 |
alilleybrinker
commented
3 months ago Rebased on upstream. Doing local tests now and may have additional fixes to keep things running. Prior security audit failed so I'd like to address that.
coveralls
commented
3 months ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| src/steps/hook.rs | 0 | 1 | 0.0% | ||
| src/steps/mod.rs | 0 | 1 | 0.0% | ||
| src/steps/replace.rs | 0 | 1 | 0.0% | ||
| src/ops/cargo.rs | 0 | 2 | 0.0% | ||
| src/steps/publish.rs | 0 | 2 | 0.0% | ||
| src/config.rs | 1 | 6 | 16.67% | ||
| src/steps/release.rs | 0 | 7 | 0.0% | ||
| src/ops/index.rs | 0 | 13 | 0.0% | ||
| <!-- | Total: | 1 | 33 | 3.03% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| src/ops/index.rs | 1 | 0.0% | ||
| src/config.rs | 4 | 26.98% | ||
| <!-- | Total: | 5 | --> |
| Totals | |
|---|---|
| Change from base Build 9616448391: | -0.01% |
| Covered Lines: | 224 |
| Relevant Lines: | 2474 |
alilleybrinker
commented
3 months ago Regarding the cargo-audit CI failure: it looks cargo-release itself and cargo-test-support are trying to link in different versions of libgit2. I can't reproduce this locally, and it's not tied to the changes here which don't modify any of the relevant dependencies which would induce this issue. I'd argue it's not something to try to resolve here, but ought to be resolved in crate-ci:master and then I can rebase to get the fix in this branch.
epage
commented
3 months ago The audit is non-blocking
coveralls
commented
3 months ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| src/steps/hook.rs | 0 | 1 | 0.0% | ||
| src/steps/mod.rs | 0 | 1 | 0.0% | ||
| src/steps/replace.rs | 0 | 1 | 0.0% | ||
| src/ops/cargo.rs | 0 | 2 | 0.0% | ||
| src/steps/publish.rs | 0 | 2 | 0.0% | ||
| src/config.rs | 1 | 6 | 16.67% | ||
| src/steps/release.rs | 0 | 7 | 0.0% | ||
| src/ops/index.rs | 0 | 13 | 0.0% | ||
| <!-- | Total: | 1 | 33 | 3.03% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| src/ops/version.rs | 1 | 83.84% | ||
| src/ops/index.rs | 1 | 0.0% | ||
| src/config.rs | 4 | 26.98% | ||
| <!-- | Total: | 6 | --> |
| Totals | |
|---|---|
| Change from base Build 9616448391: | -0.05% |
| Covered Lines: | 223 |
| Relevant Lines: | 2474 |
This commit adds the ability to select what certificate root store to use when making HTTPS connections. Previously,
cargo-releasewas configured to use the default fortame-index, which is the "webpki" root of trust maintained by Mozilla. This is a good and reasonable set of certificates, except for users on networks which substitute certificates.Substituting certificates is something enterprise networks will frequently do so they can man-in-the-middle HTTPS connections made on their network and thus maintain visibility into the network activities of their employees. In this setup, the users' devices will generally be running enterprise-managed software which replaces certificates used by public websites with ones provided by the network software the enterprise uses, with the root certificates for these substituted chains being placed in the users' local system certificate store. In that case, with only the "webpki" certificate store loaded for
cargo-release, the substituted certificates will fail to validate, and publication of new versions (indeed, even checking publication status of the crate attempting to be published) will fail with an HTTPS error about an untrusted certificate.The solution chosen here was to add a configuration element, and a CLI flag under the "publish" section, which lets the user pick between "webpki" (Mozilla) or "native" (local system) certificate trust stores. The portion of the code in
src/ops/index.rswhich handles connecting to the registry index has been updated to configure which certificate store to use based on the user's selection.The one final wrinkle is that we get
reqwest, the dependency which actually handles HTTPS connections, through thetame-indexcrate, which re-exports it. To enable the APIs inreqwestfor configuring what TLS certs to pick up, we have to enable the "native-certs" feature ontame-index, while leaving the default features on. This is somethingtame-indexnormally recommends against, because it assumes you want to exclusively activate one of them at compile time. In our case, we need the selection to happen at runtime, so we need both to be compiled in.Fixes #764