Closed victorhurdugaci closed 9 years ago
ghost
commented
9 years ago I've adjusted incoming JWT processing so issue should be fixed in latest plugin version. If not, I've logged more details so please share teamcity server logs with me if it will not work. Thank you for the feedback!
victorhurdugaci
commented
9 years ago Hi Evgeniy,
Thanks for the fast reply. Your changes didn't fix the issue.
However, I spent some time today and finally figured out the bug. The payload in id_token that comes back from the OAuth server and which is supposed to be base64 encoded, has a length that is not a multiple of 4. It seems that Base64.decodeBase64 will drop the last character in that case - the closing } was dropped, that's why the json was invalid.
The fix is trivial and I tried it on our TC server. In JWT.parse before getting the bytes from the payload](https://github.com/ekoshkin/teamcity-azure-active-directory/blob/master/azure-active-directory-server/src/main/java/org/jetbrains/teamcity/aad/JWT.java#L41), make sure the payload has a length that's multiple of 4. If not, append = characters until you reach such a length.
while (jwsPayload.length() %4 != 0) {
jwsPayload += "=";
}@victorhurdugaci Thanks for your tip! Could you please share sample id_token with me? I'm not able to reproduce described fixed behaviour in tests.
victorhurdugaci
commented
9 years ago Sorry, I will not share the id_token for privacy reasons but I managed to create a message that reproduces the problem. What is very interesting is that I can only reproduce this while running in TC. If I run this as a java console app, I don't get the same results (maybe it has something to do with what version of the libraries TC uses).
Here is a SO thread that gave me the idea about padding with "=" to make the length a multiple of 4: http://stackoverflow.com/questions/6265912/javax-xml-binds-base64-encoder-decoder-eats-last-two-characters-of-string
// Encoded with
// https://www.base64decode.org/
// VGhlcmUgc2hvdWxkIGJlIDIgZG90cyBhdCB0aGUgZW5kIG9mIHRoaXMgc3RyaW5nLi4=
// is
// "There should be 2 dots at the end of this string.." (no quotes)
// If you try both strings below in the online decoder, you should get the same result
// String length is multiple of 4 (padded with "=")
CheckDecoding("VGhlcmUgc2hvdWxkIGJlIDIgZG90cyBhdCB0aGUgZW5kIG9mIHRoaXMgc3RyaW5nLi4=");
// String length is not multiple of 4
CheckDecoding("VGhlcmUgc2hvdWxkIGJlIDIgZG90cyBhdCB0aGUgZW5kIG9mIHRoaXMgc3RyaW5nLi4");
private static void CheckDecoding(String base64str){
try {
String message="";
LOG.warn("\n---START---");
LOG.warn("\nInput: " + base64str);
LOG.warn("\nInput length: " + base64str.length());
byte[] bytes = base64str.getBytes("UTF-8");
LOG.warn("\nInput bytes length: " + bytes.length);
LOG.warn("\nInput bytes:\n");
for(int i=0;i<bytes.length;i++)
{
message += bytes[i] + " ";
}
LOG.warn(message);
byte[] decodedBytes = Base64.decodeBase64(bytes);
LOG.warn("\nDecoded length: " + decodedBytes.length);
message = "";
LOG.warn("\nDecoded bytes:\n");
for(int i=0;i<decodedBytes.length;i++)
{
message += decodedBytes[i] + " ";
}
LOG.warn(message);
String result = new String(decodedBytes, "UTF-8");
LOG.warn("\nResult length: " + result.length());
LOG.warn("\nResult:\n");
LOG.warn(result);
LOG.warn("\n---DONE---");
}
catch(Exception ex) {}
}And the results. Notice than in the second invocation, decoded bytes has one extra byte even though the input is one character shorter and the entire byte array got padded with 3 zero bytes. Also, the result dropped the two final dots.
---START---
Input: VGhlcmUgc2hvdWxkIGJlIDIgZG90cyBhdCB0aGUgZW5kIG9mIHRoaXMgc3RyaW5nLi4=
Input length: 68
Input bytes length: 68
Input bytes:
86 71 104 108 99 109 85 103 99 50 104 118 100 87 120 107 73 71 74 108 73 68 73 103 90 71 57 48 99 121 66 104 100 67 66 48 97 71 85 103 90 87 53 107 73 71 57 109 73 72 82 111 97 88 77 103 99 51 82 121 97 87 53 110 76 105 52 61
Decoded length: 50
Decoded bytes:
84 104 101 114 101 32 115 104 111 117 108 100 32 98 101 32 50 32 100 111 116 115 32 97 116 32 116 104 101 32 101 110 100 32 111 102 32 116 104 105 115 32 115 116 114 105 110 103 46 46
Result length: 50
Result:
There should be 2 dots at the end of this string..
---DONE---
---START---
Input: VGhlcmUgc2hvdWxkIGJlIDIgZG90cyBhdCB0aGUgZW5kIG9mIHRoaXMgc3RyaW5nLi4
Input length: 67
Input bytes length: 67
Input bytes:
86 71 104 108 99 109 85 103 99 50 104 118 100 87 120 107 73 71 74 108 73 68 73 103 90 71 57 48 99 121 66 104 100 67 66 48 97 71 85 103 90 87 53 107 73 71 57 109 73 72 82 111 97 88 77 103 99 51 82 121 97 87 53 110 76 105 52
Decoded length: 51
Decoded bytes:
84 104 101 114 101 32 115 104 111 117 108 100 32 98 101 32 50 32 100 111 116 115 32 97 116 32 116 104 101 32 101 110 100 32 111 102 32 116 104 105 115 32 115 116 114 105 110 103 0 0 0
Result length: 51
Result:
There should be 2 dots at the end of this string
---DONE---
erikbra
commented
9 years ago We have the same problems for some users. You can read about how this works e.g. on Wikipedia's artice on Base64: http://en.wikipedia.org/wiki/Base64 (it's under the section on Privacy Enhanced email)
After encoding the non-padded data, if two octets of the 24-bit buffer are padded-zeros, two "=" characters are appended to the output; if one octet of the 24-bit buffer is filled with padded-zeros, one "=" character is appended. This signals the decoder that the zero bits added due to padding should be excluded from the reconstructed data. This also guarantees that the encoded output length is a multiple of 4 bytes.
To sum up, three bytes in base64 must be used to represent four characters. If the base64 encoded result of the message is not a multiple of four bytes, zeros should be added to the message to make the base64 encoded version be a multiple of four. One or two zeros could be added, and should be removed from the decoded message after decoding.
As a convention (unsure if it's actually required), if one zero is added to the original message, a single '=' sign is added to the base64 encoded message. If two zeros are added, two '='s are added.
As @victorhurdugaci said above, the base64 decoder used in TeamCity assumes all base64-encoded messages are a multiple of four bytes, hence padded with '=' signs if they are not. If they aren't, the last characters of the original message will be trimmed away.
Should we create a pull request on the fix? I am not heavily into java development at the time, but could manage to make a pull request, I think.
erikbra
commented
9 years ago I added a pull request with a fix: https://github.com/ekoshkin/teamcity-azure-active-directory/pull/5
ghost
commented
9 years ago merged pull request with fix, thnks @erikbra
erikbra
commented
9 years ago Lovely.
I've enabled the plugin on a TC server but it fails to login. After I type in the credentials, I get the following error: