-
Notifications
You must be signed in to change notification settings - Fork 85
[DON'T- MERGE]Add the design for the API to help rotate secret. #259
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
Open
reasonerjt
wants to merge
2
commits into
goharbor:main
Choose a base branch
from
reasonerjt:rotate-secret-key
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
# Proposal: API to support updating the encrypted data with a new secret key | ||
|
||
Author: `reasonerjt` | ||
|
||
## Abstract | ||
|
||
This proposal aims to provide an API to support updating the encrypted data with a new secret key. It will decrypt the persisted data | ||
with the old key and use the new key to encrypt the data again. This API will be used to support rotating the secret key in Harbor. | ||
|
||
## Background | ||
|
||
Harbor needs to persist sensitive data in datastore like database, including the credentials for connecting to external services, like LDAP | ||
and other registries. These sensitive data are encrypted using symmetric methods before persisted to the database. When Harbor needs to read | ||
the data, it will decrypt the data using the same secret key, which is mounted to the "harbor-core" container or pod. | ||
Because which key was used to encrypt certain data is not tracked, there's problem when the secret key has to be changed. When a new key is | ||
configured in Harbor, the data encrypted with the old key will not be able to be decrypted. | ||
|
||
## Proposal | ||
|
||
I propose to provide a simple API to help Harbor's system admin update the encrypted data with a new secret key. The Admin will provide the | ||
old and new secret keys as parameters when calling the API, and Harbor will decrypt the data with the old key and encrypt the data with the new key, | ||
then persist the data back to the database. This API will be used to support rotating the secret key in Harbor. | ||
|
||
A typical workflow to rotate the secret key will be like this: | ||
Generate the new secret key --> Call the API to update the encrypted data with the new key --> Update the secret key and restart `harbor-core` | ||
|
||
## Non Goal | ||
|
||
- This proposal does not cover the end-to-end flow for rotating the secret key in Harbor. User may use this API in conjunction with | ||
other tools. | ||
- This proposal will not cover the user case to recover or reset the secret key in case of losing the key. | ||
- Although it is debatable whether encrypting the data is necessary, we will not refine the encryption flow in this proposal. | ||
- When the encrypted data is updated some on-going functionality maybe broken. It is the responsibility of the system admin to manage the impact, for example, | ||
call this API during the maintenance window. | ||
|
||
## Design | ||
|
||
### The encrypted data in Harbor | ||
|
||
As for v2.12.0 of Harbor, the encrypted data in Harbor includes: | ||
|
||
| Table Name | Column Name | Description | Comment | | ||
|------------|---------------|-----------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| properties | v | Some sensitive properties like LDAP search password | We can distinguish the encrypted data by checking the pattern of the string from the DB, or check the "itemType", of the metadata: [link](https://github.com/goharbor/harbor/blob/28896b1bd6916d59e2445226753c73365d9ba062/src/lib/config/metadata/metadatalist.go#L93) | | ||
| oidc_user | secret | The CLI secret for user authenticated via OIDC | | | ||
| oidc_user | token | The token data of a user | It's a serialized JSON containing the ID token and refresh Token | | ||
| registry | access_secret | The access secret of a registry endpoint | | | ||
|
||
This API MUST handle all the encrypted data in Harbor. Therefore, whenever there's a change in the scope of encrypted data, this API should be updated accordingly. | ||
|
||
### The API | ||
|
||
The API to rotate the secret key will look like | ||
``` | ||
POST /api/v2.0/system/rotatesecretkey | ||
``` | ||
Request body will be JSON Object containing the current and new secret key, and an optional attribute `skip_oidc_secret`: | ||
```json | ||
{ | ||
"current_secret_key": "current_secret", | ||
"new_secret_key": "new_secret", | ||
"skip_oidc_secret": false | ||
} | ||
``` | ||
This API will be available to system admin only, if the user is not a system admin, the API will return `403`. | ||
|
||
When it's called Harbor will decrypt the encrypted data with the current secret key and encrypt the data with the new secret | ||
key, then persist the data back to the database. Because the secret key is set via environment variable, so after the data are | ||
updated and persisted the secret key is not updated, if we continue use this key to encrypt data it will cause inconsistency and | ||
will be hard to fix. Therefore, Harbor will enter "read-only" mode after the data are updated, and the system admin should update | ||
the secret key and unset the "read-only" mode. In addition, to avoid inconsistency all these actions will be wrapped into one | ||
transaction, if any step fails the API return will return status code `500` and the transaction will be rolled back. | ||
|
||
When the attribute `skip_oidc_secret` is set to `true`, the API will skip updating the OIDC secret. This is added to avoid | ||
the corner case where there are too many records in the `oidc_user` table and the update will take too long. The system admin | ||
should call the API with `skip_oidc_secret` set to `false` first, and set it to `true` only when the API failed due to the reason | ||
mentioned. If the API is called with `skip_oidc_secret` set to `true`, the OIDC secrets will not be usable after the secret key is | ||
updated, and the user will need to manually update the OIDC secret via Harbor's UI. | ||
|
||
### Implementation | ||
|
||
The implementation of this API is relatively straight forward, the controller to handle the actual logic will call db layer directly | ||
to update the data. We should make sure there're detailed log messages to track the progress of update of each type of data. | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I found an issue when testing the code.
Harbor has been using AES256-CFB to encrypt/decrypt the data, and if the user provides an incorrect key as "current_secret_key" to decrypt the data, there seems no good way to detect it. Therefore, if the user passes a wrong key when calling the API, the wrong data will be persisted and it will be very hard to recover.
This seems a bit too dangerous even though we expect it to be used only by system admin.