feat: use scoped signing keys for users#104
feat: use scoped signing keys for users#104BZValoche wants to merge 1 commit intoWirelessCar:mainfrom
Conversation
Signed-off-by: Valery Fouques <bzflag@free.fr>
thobiaskarlsson
left a comment
thobiaskarlsson
left a comment
There was a problem hiding this comment.
Thank you for providing this. I really like the concept of Scoped Signing Keys, it's a great feature that should be the preferred way of dealing with users (as you mention). However, I have some concerns about this specific implementation of it and see great risks with how it's dealt with. See comments.
| return fmt.Errorf("failed to create secret: %w", err) | ||
| } | ||
| } else { | ||
| if !update { |
There was a problem hiding this comment.
issue: If Signing Keys are being used, hence update is true, then we will never update the secret according to this code. This also means that if meta or valueMap changes between reconciliations we won't update the secret with e.g. new labels etc. Suggestion is to remove the update bool parameter and simply always apply/update the secret. I consider this parameter to be sub-optimizing while adding unnecessary complexity. Please correct me if I'm missing something crucial here.
| if len(accountJWT) == 0 { | ||
| return nil, fmt.Errorf("account jwt for account %s not found", accountID) | ||
| } | ||
| accountNatsClaims, err := jwt.DecodeAccountClaims(accountJWT) |
There was a problem hiding this comment.
issue: To start with, I'm not that fond with both the Account- and User Manager being controllers of the Account JWT, it adds a new dimension of complexity and troubleshooting issues if they are not 100% aligned. That said, I already see an issue with this. The Update method in Account Manager create a new Account JWT Claims, from scratch, and simply upsert it. Since it doesn't consider the content of the stored JWT in NATS it will remove all those signing keys added by the User Manager. The Account will be reconciled if even a small change is being detected, like an account limit change, and then the user signing keys would be gone and all users locked out.
I really love the concept of this account delegated user permissions and what you are trying to achieve here, it totally makes sense. But it might require some major refactoring to ensure resource ownership and idempotency.
| } | ||
| accountNatsClaims.SigningKeys.Add(signingKeyPublicKey) | ||
|
|
||
| delete(accountNatsClaims.SigningKeys, signingKeyPublicKey) |
There was a problem hiding this comment.
question: Should you really be deleting the (newly created) key here?
| Claims AccountClaims `json:"claims,omitempty"` | ||
| // +listType=set | ||
| // +optional | ||
| SigningKeys []string `json:"signingKeys,omitempty"` |
There was a problem hiding this comment.
question: Correct me if I'm wrong. If a user secret is exposed, and we want to revoke that signing key to deny the credentials by either deleting it, or re-creating it (along with a new user secret). How would the process look like? What resource do I (manually) delete/modify/trigger in Kubernetes to achieve this?
| } | ||
|
|
||
| const ( | ||
| SecretNameUserSignTemplate = "%s-u-sign-%s" // #nosec G101 |
There was a problem hiding this comment.
suggestion: This constant is also defined in internal/user/constants.go. Suggestion is to keep only one of them.
| return hash | ||
| } | ||
|
|
||
| func (u *User) GetUserSigningKeySecretName() string { |
There was a problem hiding this comment.
suggestion: The signing key secret name is rather an implementation detail than related to the API model. Suggestion is to move this logic into the manager/domain instead.
| Claims AccountClaims `json:"claims,omitempty"` | ||
| // +listType=set | ||
| // +optional | ||
| SigningKeys []string `json:"signingKeys,omitempty"` |
There was a problem hiding this comment.
suggestion: My guess is that this is a []string of Signing Key public part(?). If you have an account with e.g. 20+ users, then this will be a very non-intuitive list of IDs that is hard for any human to verify upon e.g. troubleshooting permission issues. Suggestion would be to use an array of struct instead and provide more information, for instance pub key + user name/user resource reference/user DisplayName instead.
| AccountName string `json:"accountName"` | ||
| // UseSigningKey generates a scopping signing key for the user. | ||
| // +Optional | ||
| UseSigningKey bool `json:"useSigningKey,omitempty"` |
There was a problem hiding this comment.
suggestion: The NATS naming of this concept is "Scoped Signing Keys" according to the Signing Keys docs. Re-use this wording when referring to the concept, e.g. UseScopedSigningKey.
| type: Opaque | ||
| metadata: | ||
| name: operator-sau-creds | ||
| name: |
There was a problem hiding this comment.
question: Why did you remove the name here? It's a basic test case that (hopefully) should never need to be altered, to verify backward complexity etc.
There was a problem hiding this comment.
Fair enough, please rollback and close this conversation when done so. 👍
2 participants
Using scoped signing keys allow to:
This PR adds a property in the User specification, which can be set to enable the use of signing keys. If not, it works the same than before.
NATS documentation on scoper signing keys: https://docs.nats.io/using-nats/nats-tools/nsc/signing_keys
I might also have corrected some bits of code which do not relate directly with the purpose of this PR, which prevented me to run the operator locally.
Still WIP, there is room for refactor, and I probably missed some cases.