From 0636cd898f064cf1b36e153ca1ad5c99cd6b6d91 Mon Sep 17 00:00:00 2001 From: Bill Burke Date: Tue, 11 Nov 2014 21:28:35 -0500 Subject: [PATCH 1/2] fix incorrect JWS implementation --- .../org/keycloak/jose/jws/JWSBuilder.java | 117 ++++++++++++++---- .../java/org/keycloak/jose/jws/JWSInput.java | 5 + .../jose/jws/crypto/HMACProvider.java | 2 +- .../keycloak/jose/jws/crypto/RSAProvider.java | 2 +- 4 files changed, 99 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/org/keycloak/jose/jws/JWSBuilder.java b/core/src/main/java/org/keycloak/jose/jws/JWSBuilder.java index 32490f7157..3ef2d8305b 100755 --- a/core/src/main/java/org/keycloak/jose/jws/JWSBuilder.java +++ b/core/src/main/java/org/keycloak/jose/jws/JWSBuilder.java @@ -58,81 +58,148 @@ public class JWSBuilder { } } - protected String encode(Algorithm alg, byte[] data, byte[] signature) { - StringBuffer encoding = new StringBuffer(); - encoding.append(encodeHeader(alg)); + protected String encodeAll(StringBuffer encoding, byte[] signature) { encoding.append('.'); - encoding.append(Base64Url.encode(data)); - encoding.append('.'); - if (alg != Algorithm.none) { + if (signature != null) { encoding.append(Base64Url.encode(signature)); } return encoding.toString(); } + protected void encode(Algorithm alg, byte[] data, StringBuffer encoding) { + encoding.append(encodeHeader(alg)); + encoding.append('.'); + encoding.append(Base64Url.encode(data)); + } + protected byte[] marshalContent() { return contentBytes; } public class EncodingBuilder { public String none() { + StringBuffer buffer = new StringBuffer(); byte[] data = marshalContent(); - return encode(Algorithm.none, data, null); + encode(Algorithm.none, data, buffer); + return encodeAll(buffer, null); } public String rsa256(PrivateKey privateKey) { + StringBuffer buffer = new StringBuffer(); byte[] data = marshalContent(); - byte[] signature = RSAProvider.sign(data, Algorithm.RS256, privateKey); - return encode(Algorithm.RS256, data, signature); + encode(Algorithm.RS256, data, buffer); + byte[] signature = null; + try { + signature = RSAProvider.sign(buffer.toString().getBytes("UTF-8"), Algorithm.RS256, privateKey); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + return encodeAll(buffer, signature); } public String rsa384(PrivateKey privateKey) { + StringBuffer buffer = new StringBuffer(); byte[] data = marshalContent(); - byte[] signature = RSAProvider.sign(data, Algorithm.RS384, privateKey); - return encode(Algorithm.RS384, data, signature); + encode(Algorithm.RS384, data, buffer); + byte[] signature = null; + try { + signature = RSAProvider.sign(buffer.toString().getBytes("UTF-8"), Algorithm.RS384, privateKey); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + return encodeAll(buffer, signature); } public String rsa512(PrivateKey privateKey) { + StringBuffer buffer = new StringBuffer(); byte[] data = marshalContent(); - byte[] signature = RSAProvider.sign(data, Algorithm.RS512, privateKey); - return encode(Algorithm.RS512, data, signature); + encode(Algorithm.RS512, data, buffer); + byte[] signature = null; + try { + signature = RSAProvider.sign(buffer.toString().getBytes("UTF-8"), Algorithm.RS512, privateKey); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + return encodeAll(buffer, signature); } public String hmac256(byte[] sharedSecret) { + StringBuffer buffer = new StringBuffer(); byte[] data = marshalContent(); - byte[] signature = HMACProvider.sign(data, Algorithm.HS256, sharedSecret); - return encode(Algorithm.HS256, data, signature); + encode(Algorithm.HS256, data, buffer); + byte[] signature = null; + try { + signature = HMACProvider.sign(buffer.toString().getBytes("UTF-8"), Algorithm.HS256, sharedSecret); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + return encodeAll(buffer, signature); } public String hmac384(byte[] sharedSecret) { + StringBuffer buffer = new StringBuffer(); byte[] data = marshalContent(); - byte[] signature = HMACProvider.sign(data, Algorithm.HS384, sharedSecret); - return encode(Algorithm.HS384, data, signature); + encode(Algorithm.HS384, data, buffer); + byte[] signature = null; + try { + signature = HMACProvider.sign(buffer.toString().getBytes("UTF-8"), Algorithm.HS384, sharedSecret); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + return encodeAll(buffer, signature); } public String hmac512(byte[] sharedSecret) { + StringBuffer buffer = new StringBuffer(); byte[] data = marshalContent(); - byte[] signature = HMACProvider.sign(data, Algorithm.HS512, sharedSecret); - return encode(Algorithm.HS512, data, signature); + encode(Algorithm.HS512, data, buffer); + byte[] signature = null; + try { + signature = HMACProvider.sign(buffer.toString().getBytes("UTF-8"), Algorithm.HS512, sharedSecret); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + return encodeAll(buffer, signature); } public String hmac256(SecretKey sharedSecret) { + StringBuffer buffer = new StringBuffer(); byte[] data = marshalContent(); - byte[] signature = HMACProvider.sign(data, Algorithm.HS256, sharedSecret); - return encode(Algorithm.HS256, data, signature); + encode(Algorithm.HS256, data, buffer); + byte[] signature = null; + try { + signature = HMACProvider.sign(buffer.toString().getBytes("UTF-8"), Algorithm.HS256, sharedSecret); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + return encodeAll(buffer, signature); } public String hmac384(SecretKey sharedSecret) { + StringBuffer buffer = new StringBuffer(); byte[] data = marshalContent(); - byte[] signature = HMACProvider.sign(data, Algorithm.HS384, sharedSecret); - return encode(Algorithm.HS384, data, signature); + encode(Algorithm.HS384, data, buffer); + byte[] signature = null; + try { + signature = HMACProvider.sign(buffer.toString().getBytes("UTF-8"), Algorithm.HS384, sharedSecret); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + return encodeAll(buffer, signature); } public String hmac512(SecretKey sharedSecret) { + StringBuffer buffer = new StringBuffer(); byte[] data = marshalContent(); - byte[] signature = HMACProvider.sign(data, Algorithm.HS512, sharedSecret); - return encode(Algorithm.HS512, data, signature); + encode(Algorithm.HS512, data, buffer); + byte[] signature = null; + try { + signature = HMACProvider.sign(buffer.toString().getBytes("UTF-8"), Algorithm.HS512, sharedSecret); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + return encodeAll(buffer, signature); } } } diff --git a/core/src/main/java/org/keycloak/jose/jws/JWSInput.java b/core/src/main/java/org/keycloak/jose/jws/JWSInput.java index f86151f5b1..6db10fa077 100755 --- a/core/src/main/java/org/keycloak/jose/jws/JWSInput.java +++ b/core/src/main/java/org/keycloak/jose/jws/JWSInput.java @@ -15,6 +15,7 @@ public class JWSInput { String encodedHeader; String encodedContent; String encodedSignature; + String encodedSignatureInput; JWSHeader header; byte[] content; byte[] signature; @@ -26,6 +27,7 @@ public class JWSInput { if (parts.length < 2 || parts.length > 3) throw new IllegalArgumentException("Parsing error"); encodedHeader = parts[0]; encodedContent = parts[1]; + encodedSignatureInput = encodedHeader + '.' + encodedContent; try { content = Base64Url.decode(encodedContent); if (parts.length > 2) { @@ -55,6 +57,9 @@ public class JWSInput { public String getEncodedSignature() { return encodedSignature; } + public String getEncodedSignatureInput() { + return encodedSignatureInput; + } public JWSHeader getHeader() { return header; diff --git a/core/src/main/java/org/keycloak/jose/jws/crypto/HMACProvider.java b/core/src/main/java/org/keycloak/jose/jws/crypto/HMACProvider.java index f73f14e9da..4772280540 100755 --- a/core/src/main/java/org/keycloak/jose/jws/crypto/HMACProvider.java +++ b/core/src/main/java/org/keycloak/jose/jws/crypto/HMACProvider.java @@ -74,7 +74,7 @@ public class HMACProvider { public static boolean verify(JWSInput input, byte[] sharedSecret) { try { - byte[] signature = sign(input.getContent(), input.getHeader().getAlgorithm(), sharedSecret); + byte[] signature = sign(input.getEncodedSignatureInput().getBytes("UTF-8"), input.getHeader().getAlgorithm(), sharedSecret); String x = Base64Url.encode(signature); return x.equals(input.getEncodedSignature()); } catch (Exception e) { diff --git a/core/src/main/java/org/keycloak/jose/jws/crypto/RSAProvider.java b/core/src/main/java/org/keycloak/jose/jws/crypto/RSAProvider.java index 151d35475f..42baccffe6 100755 --- a/core/src/main/java/org/keycloak/jose/jws/crypto/RSAProvider.java +++ b/core/src/main/java/org/keycloak/jose/jws/crypto/RSAProvider.java @@ -49,7 +49,7 @@ public class RSAProvider { try { Signature verifier = getSignature(input.getHeader().getAlgorithm()); verifier.initVerify(publicKey); - verifier.update(input.getContent()); + verifier.update(input.getEncodedSignatureInput().getBytes("UTF-8")); return verifier.verify(input.getSignature()); } catch (Exception e) { throw new RuntimeException(e); From 5c6dd8e0c3c1d2cd82f271a1df65d62264dd10f3 Mon Sep 17 00:00:00 2001 From: Bill Burke Date: Tue, 11 Nov 2014 22:09:38 -0500 Subject: [PATCH 2/2] temp fix for contributor SAML PR --- .../src/main/java/org/keycloak/protocol/saml/SamlProtocol.java | 2 +- .../test/java/org/keycloak/testsuite/saml/SamlBindingTest.java | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/saml/saml-protocol/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java b/saml/saml-protocol/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java index 1d0367afdc..e8b15046b6 100755 --- a/saml/saml-protocol/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java +++ b/saml/saml-protocol/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java @@ -128,7 +128,7 @@ public class SamlProtocol implements LoginProtocol { if(nameIdFormat.equals(JBossSAMLURIConstants.NAMEID_FORMAT_EMAIL.get())) { return userSession.getUser().getEmail(); } else if(nameIdFormat.equals(JBossSAMLURIConstants.NAMEID_FORMAT_TRANSIENT.get())) { - return clientSession.getNote(ClientSessionCode.ACTION_KEY); + return userSession.getUser().getUsername(); } else { // TODO: Support for persistent NameID (pseudo-random identifier persisted in user object) return userSession.getUser().getUsername(); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/saml/SamlBindingTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/saml/SamlBindingTest.java index 6c104fbbed..94d65ad8dc 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/saml/SamlBindingTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/saml/SamlBindingTest.java @@ -83,6 +83,7 @@ public class SamlBindingTest { Assert.assertEquals(driver.getCurrentUrl(), "http://localhost:8081/auth/realms/demo/protocol/saml"); loginPage.login("bburke", "password"); Assert.assertEquals(driver.getCurrentUrl(), "http://localhost:8081/sales-post/"); + System.out.println(driver.getPageSource()); Assert.assertTrue(driver.getPageSource().contains("bburke")); driver.navigate().to("http://localhost:8081/sales-post?GLO=true"); Assert.assertEquals(driver.getCurrentUrl(), "http://localhost:8081/auth/realms/demo/protocol/saml");