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

ROX-18826: watch jwt-key mount changes #7322

Closed
wants to merge 15 commits into from

Conversation

ivan-degtiarenko
Copy link
Contributor

@ivan-degtiarenko ivan-degtiarenko commented Aug 8, 2023

Description

The goal of this PR is to make Central listen to jwt-key updates in central-tls secret. This PR includes:

  • listening to jwt-key modifications
  • updating corresponding JWT token validator so that it uses actual value of public key
  • updating corresponding token issuer so that it uses actual value of private key

Checklist

  • Investigated and inspected CI test results
  • Unit test and regression tests added
  • Evaluated and added CHANGELOG entry if required
  • Determined and documented upgrade steps
  • Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

Manually execute with port-forward located on port 8000 pointing to central:

  1. Create API Token old-token
  2. In terminal export ROX_API_TOKEN=<old-token-value
  3. roxctl central debug download-diagnostics --endpoint=localhost:8000 should be successful
  4. export JWT_KEY=<valid jwt key>
  5. kubectl patch secret central-tls -n stackrox --patch="{\"data\": { \"jwt-key.pem\": \"$JWT_KEY\" }}"
  6. roxctl central debug download-diagnostics --endpoint=localhost:8000 should return an error
  7. Create API Token new-token
  8. In terminal export ROX_API_TOKEN=<new-token-value
  9. roxctl central debug download-diagnostics --endpoint=localhost:8000 should be successful

@ivan-degtiarenko ivan-degtiarenko requested a review from a team as a code owner August 8, 2023 12:38
@ivan-degtiarenko ivan-degtiarenko removed the request for review from a team August 8, 2023 12:39
@ghost
Copy link

ghost commented Aug 8, 2023

Images are ready for the commit at cc71cfd.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.1.x-693-gcc71cfd62b.

central/jwt/jwt.go Outdated Show resolved Hide resolved
pkg/jwt/keywatch.go Outdated Show resolved Hide resolved
pkg/jwt/private_keys.go Outdated Show resolved Hide resolved
pkg/jwt/public_keys.go Outdated Show resolved Hide resolved
pkg/jwt/keywatch.go Outdated Show resolved Hide resolved
central/jwt/jwt.go Outdated Show resolved Hide resolved
pkg/jwt/private_keys.go Show resolved Hide resolved
pkg/auth/tokens/issuer_factory.go Outdated Show resolved Hide resolved
}

// We pass parameter here to satisfy WatchPrivateKeyDir interface.
func loadPrivateKey(_ string) (*rsa.PrivateKey, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: to potentially circumvent adding the parameter here, we may do the same inline in l.65:

	jwt.WatchPrivateKeyDir(privateKeyDir, func(_ string) (*rsa.PrivateKey, error) { return loadPrivateKey() }, func(key *rsa.PrivateKey) {
		privateKeyStore.UpdateKey(keyID, key)
	})

Feel like this is more clean instead of passing privateKeyDir to loadPrivateKey in l.59.

pkg/jwt/keywatch.go Outdated Show resolved Hide resolved
pkg/jwt/public_keys.go Outdated Show resolved Hide resolved
Copy link
Contributor

@parametalol parametalol left a comment

Choose a reason for hiding this comment

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

LGTM, with code style comments.

Comment on lines 23 to 32
// CreateRS256SignerAndValidator creates a token signer and validator pair with the given properties from the
// specified RSA private key.
func CreateRS256SignerAndValidator(issuerID string, audience jwt.Audience, privateKeyStore PrivateKeyGetter, publicKeyStore PublicKeyGetter, keyID string) (*SignerFactory, Validator) {
validator := NewRS256Validator(publicKeyStore, issuerID, audience)
signerFactory := &SignerFactory{
keyStore: privateKeyStore,
keyID: keyID,
}
return signer, validator, nil
return signerFactory, validator
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This CreateRS256SignerAndValidator function takes two sets of parameters, which are used to construct two objects.
The parameters for the returned objects constructors do not overlap, so I'd suggest splitting this function into CreateSignerFactory (sic!) and CreateRS256Validator with the corresponding parameters. And then we would realize that one doesn't need another function to create a Validator.

@@ -40,8 +43,8 @@ func getBytesFromPem(path string) ([]byte, error) {

// GetPrivateKeyBytes returns the contents of the file containing the private key.
func GetPrivateKeyBytes() ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func GetPrivateKeyBytes() ([]byte, error) {
func GetPrivateKeyBytes(dir string) ([]byte, error) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately this method is also used here:

jwtKey, err := jwt.GetPrivateKeyBytes()

which means we would either have to copy and reuse same parameter there as well or, even worse, introduce second method with different name. That's why I decided to keep it as it is

central/jwt/jwt.go Outdated Show resolved Hide resolved
central/jwt/jwt.go Show resolved Hide resolved
@ivan-degtiarenko
Copy link
Contributor Author

/retest

1 similar comment
@ivan-degtiarenko
Copy link
Contributor Author

/retest

@ivan-degtiarenko
Copy link
Contributor Author

Decided to gather more information from the customer as the suggested approach does not seem to solve their problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants