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

Backport of PKI - Fix order of chain building writes into release/1.11.x #17805

Conversation

hc-github-team-secure-vault-core
Copy link
Collaborator

Backport

This PR is auto-generated from #17772 to be assessed for backporting due to the inclusion of the label backport/1.11.x.

WARNING automatic cherry-pick of commits failed. Commits will require human attention.

merge conflict error: POST https://api.github.com/repos/hashicorp/vault/merges: 409 Merge conflict []

The below text is copied from the body of the original PR.


Probably best to defer to the commit messages here:

commit 8010a8755755afd19d5a62fd5073acb2fea1e1ba
Author: Alexander Scheel <alex.scheel@hashicorp.com>
Date:   Wed Nov 2 08:59:44 2022 -0400

    Remigrate ca_chains to fix any missing issuers
    
    In the previous commit, we identified an issue that would occur on
    legacy issuer migration to the new storage format. This is easy enough
    to detect for any given mount (by an operator), but automating scanning
    and remediating all PKI mounts in large deployments might be difficult.
    
    Write a new storage migration version to regenerate all chains on
    upgrade, once.
    
    Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

commit 8a786b0c57c27a6e3dc1d06edacd03672d8f8bc9
Author: Alexander Scheel <alex.scheel@hashicorp.com>
Date:   Wed Nov 2 08:12:34 2022 -0400

    Ensure correct write ordering in rebuildIssuersChains
    
    When troubleshooting a recent migration failure from 1.10->1.11, it was
    noted that some PKI mounts had bad chain construction despite having
    valid, chaining issuers. Due to the cluster's leadership trashing
    between nodes, the migration logic was re-executed several times,
    partially succeeding each time. While the legacy CA bundle migration
    logic was written with this in mind, one shortcoming in the chain
    building code lead us to truncate the ca_chain: by sorting the list of
    issuers after including non-written issuers (with random IDs), these
    issuers would occasionally be persisted prior to storage _prior_ to
    existing CAs with modified chains.
    
    The migration code carefully imported the active issuer prior to its
    parents. However, due to this bug, there was a chance that, if write to
    the pending parent succeeded but updating the active issuer didn't, the
    active issuer's ca_chain field would only contain the self-reference and
    not the parent's reference as well. Ultimately, a workaround of setting
    and subsequently unsetting a manual chain would force a chain
    regeneration.
    
    In this patch, we simply fix the write ordering: because we need to
    ensure a stable chain sorting, we leave the sort location in the same
    place, but delay writing the provided referenceCert to the last
    position. This is because the reference is meant to be the user-facing
    action: without transactional write capabilities, other chains may
    succeed, but if the last user-facing action fails, the user will
    hopefully retry the action. This will also correct migration, by
    ensuring the subsequent issuer import will be attempted again,
    triggering another chain build and only persisting this issuer when
    all other issuers have also been updated.
    
    Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

As noted above, patching the issuer to set manual_chain=self followed by manual_chain= will be an adequate workaround.


Overview of commits

@hc-github-team-secure-vault-core hc-github-team-secure-vault-core force-pushed the backport/cipherboy-fix-order-of-chain-building-writes/unduly-close-turtle branch from f07857b to 0fec35b Compare November 3, 2022 15:50
@hashicorp-cla
Copy link

hashicorp-cla commented Nov 3, 2022

CLA assistant check
All committers have signed the CLA.

Co-authored-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@cipherboy cipherboy force-pushed the backport/cipherboy-fix-order-of-chain-building-writes/unduly-close-turtle branch from 3ad1d77 to c3c64e0 Compare November 9, 2022 15:14
@cipherboy cipherboy marked this pull request as ready for review November 9, 2022 15:14
@cipherboy cipherboy enabled auto-merge (squash) November 9, 2022 15:14
@cipherboy cipherboy merged commit 0520326 into release/1.11.x Nov 9, 2022
@cipherboy cipherboy deleted the backport/cipherboy-fix-order-of-chain-building-writes/unduly-close-turtle branch December 1, 2022 14:56
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.

None yet

3 participants