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

Fix keyring file missing after Vault restart #15946

Merged
merged 4 commits into from
Jun 15, 2022

Conversation

shujun10086
Copy link
Contributor

@shujun10086 shujun10086 commented Jun 11, 2022

Fix keyring file missing after Vault restart

Vault can be killed by signal SIGTERM in anytime. If Vault is
updating keyring file and there is no byte writen into the fd
at that time,  then the size of the keyring file will be zero. 
Because the file is opened with os.O_TRUNC, and the fd will be closed
automatically after Vault exits.
Then try to unseal Vault after it startup again, the keyring file will
be deleted in getInternal function due to its size is zero. So that is
why the keyring file is missing. And the Vault cannot be unsealed
anymore due to the file missing.
Write data into a temp file then move it instead of target file directly
can avoid the issue.

Fix:#15680

Fix keyring file missing after Vault restart

    Vault can be killed by signal SIGTERM in anytime. If Vault is
    doing keyring rotation and the fd is still not closed at that time,
    the size of the keyring file will be zero. The orignal key will be
    lost totally.
    Because the file is opened with os.O_TRUNC, and the fd will be closed
    automatically after Vault exits.
    Then try to unseal Vault after it startup again, the keyring file will
    be deleted in getInternal function due to its size is zero. So that is
    why the keyring file is missing. And the Vault cannot be unsealed
    anymore due to the file missing.
    Write data into a temp file then move it can avoid the file crash.

    Fix:hashicorp#15680
@hashicorp-cla
Copy link

hashicorp-cla commented Jun 11, 2022

CLA assistant check
All committers have signed the CLA.

@shujun10086 shujun10086 marked this pull request as draft June 11, 2022 09:56
@shujun10086 shujun10086 marked this pull request as ready for review June 11, 2022 09:59
@shujun10086 shujun10086 marked this pull request as draft June 11, 2022 09:59
@shujun10086 shujun10086 marked this pull request as ready for review June 11, 2022 09:59
@shujun10086
Copy link
Contributor Author

How to add maintainer ?

@cipherboy cipherboy requested a review from raskchanky June 14, 2022 13:29
@cipherboy cipherboy added bug Used to indicate a potential bug core/seal labels Jun 14, 2022
Copy link
Contributor

@raskchanky raskchanky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I had one small suggestion for your changelog entry.

@@ -0,0 +1,3 @@
```release-note:bug
core/seal: write a temp file and encrypt it, then rename it to the target path file. This can avoid the orignal file crash if Vault restart.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changelog entry should indicate what changed from a user's perspective. For example, in this case, maybe something like:

Fix possible keyring truncation when using the file backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I am new in github, If I still needs to update the file in my repository (Vault fork project) ? How can I squash this pull request(There are three commits so far). It seems no branch name and I have no permission.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway I update the file in the fouth commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no need to worry about squashing. That will happen automatically when the PR is merged.

Copy link
Contributor

@raskchanky raskchanky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for working on this!

@raskchanky raskchanky merged commit 26258a7 into hashicorp:main Jun 15, 2022
@shujun10086
Copy link
Contributor Author

@raskchanky If this change can be ported back to other release like 1.8 1,9 ? Because this is the version we are using and meet the problem. I do not know how to port back it and if I have permission I can do it.

@raskchanky
Copy link
Contributor

@shujun10086 We support n-2 versions, so with 1.11 just released, this can be backported to 1.10 and 1.9. I can handle that for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug core/seal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vault cannot be unsealed with error "Vault is not initialized" But It is already initialized
4 participants