From 566eb14da1693a94a00569eab14d370c99b83ac7 Mon Sep 17 00:00:00 2001 From: Abhijeet V <31417623+abvaidya@users.noreply.github.com> Date: Thu, 17 Oct 2024 12:37:57 -0700 Subject: [PATCH] defer accesstoken error logging Signed-off-by: Abhijeet V <31417623+abvaidya@users.noreply.github.com> --- .../yahoo/athenz/auth/token/AccessToken.java | 43 +++++++++++-------- .../athenz/auth/token/AccessTokenTest.java | 16 +++---- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/libs/java/auth_core/src/main/java/com/yahoo/athenz/auth/token/AccessToken.java b/libs/java/auth_core/src/main/java/com/yahoo/athenz/auth/token/AccessToken.java index 53064969d27..acc69525597 100644 --- a/libs/java/auth_core/src/main/java/com/yahoo/athenz/auth/token/AccessToken.java +++ b/libs/java/auth_core/src/main/java/com/yahoo/athenz/auth/token/AccessToken.java @@ -337,23 +337,23 @@ public boolean confirmMTLSBoundToken(X509Certificate x509Cert, final String x509 // check if the certificate principal matches and the // creation time for our cert is within our configured // offset timeouts - - if (confirmX509CertPrincipal(x509Cert, cn)) { + StringBuilder errorStore = new StringBuilder(); + if (confirmX509CertPrincipal(x509Cert, cn, errorStore)) { return true; } // check if we have authorization details specified for // proxy access thus we can validate based on that - if (confirmX509CertPrincipalAuthzDetails(x509Cert)) { + if (confirmX509CertPrincipalAuthzDetails(x509Cert, errorStore)) { return true; } // direct comparison of certificate cn and provided hash - return confirmX509ProxyPrincipal(cn, x509CertHash, cnfHash); + return confirmX509ProxyPrincipal(cn, x509CertHash, cnfHash, errorStore); } - boolean confirmX509CertPrincipalAuthzDetails(X509Certificate cert) { + boolean confirmX509CertPrincipalAuthzDetails(X509Certificate cert, StringBuilder errorStore) { // make sure we have valid proxy principals specified @@ -361,7 +361,7 @@ boolean confirmX509CertPrincipalAuthzDetails(X509Certificate cert) { try { spiffeUris = (List) confirm.get(CLAIM_CONFIRM_PROXY_SPIFFE); } catch (Exception ex) { - LOG.error("Unable to parse proxy principal claim: {}", ex.getMessage()); + errorStore.append("Unable to parse proxy principal claim: ").append(ex.getMessage()); return false; } if (spiffeUris == null) { @@ -374,7 +374,8 @@ boolean confirmX509CertPrincipalAuthzDetails(X509Certificate cert) { return true; } } - + errorStore.append("confirmX509CertPrincipalAuthzDetails: cert spiffe uri: ").append(certSpiffeUri) + .append(" not found in access token proxy principals: ").append(spiffeUris); return false; } @@ -384,34 +385,37 @@ boolean confirmX509CertHash(X509Certificate cert, final String cnfHash) { return cnfHash.equals(certHash); } - boolean confirmX509ProxyPrincipal(final String cn, final String certHash, final String cnfHash) { + boolean confirmX509ProxyPrincipal(final String cn, final String certHash, final String cnfHash, StringBuilder errorStore) { // if the proxy principal set is not null then the client // has specified some value so we'll enforce it (even if // the set is empty thus rejecting all requests) - if (ACCESS_TOKEN_PROXY_PRINCIPALS != null && !ACCESS_TOKEN_PROXY_PRINCIPALS.contains(cn)) { - LOG.error("confirmX509ProxyPrincipal: unauthorized proxy principal: {}", cn); + errorStore.append("confirmX509ProxyPrincipal: unauthorized proxy principal: ").append(cn); + LOG.error(errorStore.toString()); return false; } - - return cnfHash.equals(certHash); + if (!cnfHash.equals(certHash)) { + LOG.error(errorStore.toString()); + return false; + } + return true; } - boolean confirmX509CertPrincipal(X509Certificate cert, final String cn) { + boolean confirmX509CertPrincipal(X509Certificate cert, final String cn, StringBuilder errorStore) { // if our offset is 0 then the additional confirmation // check is disabled if (ACCESS_TOKEN_CERT_OFFSET == 0) { - LOG.error("confirmX509CertPrincipal: check disabled"); + errorStore.append("confirmX509CertPrincipal: check disabled"); return false; } // our principal cn must be the client in the token if (!cn.equals(clientId)) { - LOG.error("confirmX509CertPrincipal: Principal mismatch {} vs {}", cn, clientId); + errorStore.append("confirmX509CertPrincipal: Principal mismatch").append(cn).append(" vs ").append(clientId); return false; } @@ -431,8 +435,8 @@ boolean confirmX509CertPrincipal(X509Certificate cert, final String cn) { long certIssueTime = Crypto.extractX509CertIssueTime(cert); if (certIssueTime < issueTime - 3600) { - LOG.error("confirmX509CertPrincipal: Certificate: {} issued before token: {}", - certIssueTime, issueTime); + errorStore.append("confirmX509CertPrincipal: Certificate ").append(certIssueTime) + .append(" issued before token: ").append(issueTime); return false; } @@ -442,8 +446,9 @@ boolean confirmX509CertPrincipal(X509Certificate cert, final String cn) { // need to take into account that extra hour if (certIssueTime > issueTime + ACCESS_TOKEN_CERT_OFFSET - 3600) { - LOG.error("confirmX509CertPrincipal: Certificate: {} past configured offset {} for token: {}", - certIssueTime, ACCESS_TOKEN_CERT_OFFSET, issueTime); + errorStore.append("confirmX509CertPrincipal: Certificate ").append(certIssueTime) + .append(" past configured offset: ").append(ACCESS_TOKEN_CERT_OFFSET) + .append(" for token: ").append(issueTime); return false; } diff --git a/libs/java/auth_core/src/test/java/com/yahoo/athenz/auth/token/AccessTokenTest.java b/libs/java/auth_core/src/test/java/com/yahoo/athenz/auth/token/AccessTokenTest.java index 101771cff6d..bdd7a186841 100644 --- a/libs/java/auth_core/src/test/java/com/yahoo/athenz/auth/token/AccessTokenTest.java +++ b/libs/java/auth_core/src/test/java/com/yahoo/athenz/auth/token/AccessTokenTest.java @@ -728,7 +728,7 @@ public void testConfirmX509CertPrincipalNullCert() { long now = System.currentTimeMillis() / 1000; AccessToken accessToken = createAccessToken(now); - assertFalse(accessToken.confirmX509CertPrincipal(null, "athenz.proxy")); + assertFalse(accessToken.confirmX509CertPrincipal(null, "athenz.proxy", new StringBuilder())); } @Test @@ -740,7 +740,7 @@ public void testConfirmX509CertPrincipalCertNoCN() throws IOException { Path path = Paths.get("src/test/resources/no_cn_x509.cert"); String certStr = new String(Files.readAllBytes(path)); X509Certificate cert = Crypto.loadX509Certificate(certStr); - assertFalse(accessToken.confirmX509CertPrincipal(cert, "athenz.proxy")); + assertFalse(accessToken.confirmX509CertPrincipal(cert, "athenz.proxy", new StringBuilder())); } @Test @@ -752,7 +752,7 @@ public void testConfirmX509CertPrincipalCertCNMismatch() throws IOException { Path path = Paths.get("src/test/resources/rsa_public_x509.cert"); String certStr = new String(Files.readAllBytes(path)); X509Certificate cert = Crypto.loadX509Certificate(certStr); - assertFalse(accessToken.confirmX509CertPrincipal(cert, "athenz.proxy")); + assertFalse(accessToken.confirmX509CertPrincipal(cert, "athenz.proxy", new StringBuilder())); } @Test @@ -976,7 +976,7 @@ public void testConfirmX509CertPrincipalCertStartTime() throws IOException { Path path = Paths.get("src/test/resources/mtls_token2_spec.cert"); String certStr = new String(Files.readAllBytes(path)); X509Certificate cert = Crypto.loadX509Certificate(certStr); - assertFalse(accessToken.confirmX509CertPrincipal(cert, "mtls")); + assertFalse(accessToken.confirmX509CertPrincipal(cert, "mtls", new StringBuilder())); } @Test @@ -992,7 +992,7 @@ public void testConfirmX509CertPrincipalCertStartTimeCheckDisabled() throws IOEx Path path = Paths.get("src/test/resources/mtls_token2_spec.cert"); String certStr = new String(Files.readAllBytes(path)); X509Certificate cert = Crypto.loadX509Certificate(certStr); - assertTrue(accessToken.confirmX509CertPrincipal(cert, "mtls")); + assertTrue(accessToken.confirmX509CertPrincipal(cert, "mtls", new StringBuilder())); AccessToken.setAccessTokenCertOffset(3600); } @@ -1008,7 +1008,7 @@ public void testConfirmX509CertPrincipalCertStartTimePassOffset() throws IOExcep Path path = Paths.get("src/test/resources/mtls_token2_spec.cert"); String certStr = new String(Files.readAllBytes(path)); X509Certificate cert = Crypto.loadX509Certificate(certStr); - assertFalse(accessToken.confirmX509CertPrincipal(cert, "mtls")); + assertFalse(accessToken.confirmX509CertPrincipal(cert, "mtls", new StringBuilder())); } @Test @@ -1022,7 +1022,7 @@ public void testConfirmX509CertPrincipal() throws IOException { Path path = Paths.get("src/test/resources/mtls_token2_spec.cert"); String certStr = new String(Files.readAllBytes(path)); X509Certificate cert = Crypto.loadX509Certificate(certStr); - assertTrue(accessToken.confirmX509CertPrincipal(cert, "mtls")); + assertTrue(accessToken.confirmX509CertPrincipal(cert, "mtls", new StringBuilder())); } @Test @@ -1038,7 +1038,7 @@ public void testConfirmX509CertPrincipalDisable() throws IOException { Path path = Paths.get("src/test/resources/mtls_token2_spec.cert"); String certStr = new String(Files.readAllBytes(path)); X509Certificate cert = Crypto.loadX509Certificate(certStr); - assertFalse(accessToken.confirmX509CertPrincipal(cert, "mtls")); + assertFalse(accessToken.confirmX509CertPrincipal(cert, "mtls", new StringBuilder())); AccessToken.setAccessTokenCertOffset(3600); }