Don't fallback to cert when given invalid token #9617
Conversation
| @@ -359,7 +361,7 @@ func newAuthenticator(config configapi.MasterConfig, restOptionsGetter restoptio | |||
| publicKeys = append(publicKeys, publicKey) | |||
| } | |||
| tokenAuthenticator := serviceaccount.JWTTokenAuthenticator(publicKeys, true, tokenGetter) | |||
There was a problem hiding this comment.
for sanity, name the local vars something specific like serviceAccountTokenAuthenticator here, or oauthTokenAuthenticator/oauthTokenRequestAuthenticators below
|
@liggitt I made the requested changes. |
| // token: noToken, | ||
| // cert: invalidCert, | ||
| // errorExpected: true, | ||
| // errorString: anonymousError, // TODO should this be unauthorizedError? |
There was a problem hiding this comment.
Should be unauthorized error once we figure out how to actually test the scenario
|
[test] |
|
Flake? [test] |
|
@liggitt No idea why that test failed, it works fine locally. |
|
flake on #9638 |
| defer os.RemoveAll(fakecadir) | ||
| cacerts, err := util.CertificatesFromFile(masterOptions.ServingInfo.ClientCA) | ||
| if err != nil || len(cacerts) != 1 { | ||
| t.Fatalf("Unexpected error: %v", err) |
There was a problem hiding this comment.
Technically this could give an incorrect error message when err == nil but len(cacerts) != 1.
|
Evaluated for origin test up to a9fe20d |
|
LGTM, [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5731/) (Image: devenv-rhel7_4509) |
|
Evaluated for origin merge up to a9fe20d |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5689/) |
Fixes #4537
There are three variations for both token and cert: valid, invalid and not provided. This gives a total of nine combinations. The only combination that this PR changes is with an invalid token and a valid cert. The old behavior would authenticate using the cert; the new behavior considers this an error.
Both implementations currently return the anonymous user when no token and an invalid cert are provided. It is unclear if this is the correct behavior or if an error should be returned instead.