JFROG refresh token rule #475
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #475 +/- ##
==========================================
- Coverage 90.73% 90.72% -0.01%
==========================================
Files 125 126 +1
Lines 4252 4280 +28
Branches 674 679 +5
==========================================
+ Hits 3858 3883 +25
- Misses 261 262 +1
- Partials 133 135 +2 ☔ View full report in Codecov by Sentry. |
babenek
requested review from
xDizzix,
Yullia,
iuriimet,
Dmitriy-NK,
csh519 and
kmnls
| # reftkn:01:0123456789:abcdefGhijklmnoPqrstuVwxyz0 | ||
| self._pattern = re.compile(r"reftkn:\d+:\d+:[\w_/+-]+") |
There was a problem hiding this comment.
Can we sure it is refresh token of Jfrog only with this pattern?
I don't know well, but reftkn can be used other cases also..
If you know about the Jfrog token please let me know.
There was a problem hiding this comment.
I assume, the decoded data matched for the pattern is the token type.
At least, the provider encodes their token with the approach.
Unfortunately, there is no info what's structure inside must be. Suppose, last part must have strong entropy.
https://jfrog.com/help/r/platform-api-key-deprecation-and-the-new-reference-tokens/what-is-a-reference-token
There was a problem hiding this comment.
It seems reference token.. not refresh token.
I think refresh token is more close with below link.
https://jfrog.com/help/r/jfrog-rest-apis/refresh-token
As I understood, refresh token can refresh the access token even if i don't have access token.
So yes, it is credential, but there is no clue of this refresh token's structure..
Could you check this more?
| # the check only for correct size decoding | ||
| if 54 == len(decoded): | ||
| # API key (deprecated) - a good integrity check solution was not found |
There was a problem hiding this comment.
Could you please explain why 54 is used for the check?
Following your mail there is only limited information on link.
https://github.com/jfrog/build-info/blob/17ba6912ab9cacea83defea1ca2dbd5fd4ab53e7/build-info-extractor/src/main/java/org/jfrog/build/extractor/packageManager/PackageManagerUtils.java#L24
There are some prefixes and minimal lengths, but nothing about 54..
There was a problem hiding this comment.
54 is length of decoded bytes. All tokens i found (except obviously test-like) are base58 encoding.
There is a lite "validation" in case of wrong encoding - an exception filters the value
There was a problem hiding this comment.
Hmm... I can't understand enough...
Each tokens with BASE58 encoding are having more than 73 length and it means decoded strings can have various length.. isn't it?
There was a problem hiding this comment.
The token type has 73 symbols length which decodes to 54 bytes via base58 (base64 is not acceptable due left 4 bits).
So, my suggestion is the base58 decoding should produce 54 bytes from the token (experience from found tokens).
There was a problem hiding this comment.
Yup BASE58 string which has 73 length is decoded to 54 bytes, but there are not only 73 length of tokens as you can see the JFrog code, it is more than..
So what about change the if condition to <=?
There was a problem hiding this comment.
As soon the API key is deprecated, IMO the check was designed for an extension, but it will not realized. So, the size of token now used is exactly 73. bigger size may produce false positives in many base64 encodings. "AKCp" is a short signature unfortunately.
There was a problem hiding this comment.
Okay. I got it.
Thank you.
| if self._pattern.match(decoded.decode(ASCII)): | ||
| # identity token | ||
| return False | ||
| if value.startswith("AKCp"): |
There was a problem hiding this comment.
As I checked the link the prefix from JFrog was AKCp8..
Does it wrong? Actually I already saw other token which you sent by email which starts with AKCpB.
If you have other reason to use AKCp as prefix, please let me know.
There was a problem hiding this comment.
Yes, i found 3 variants. 2,8,B. Anyway, the "if" is for detecting type of token only. It distinguishes "cmVmdGtuO" prefix and "AKCp".
Description
Please include a summary of the change and which is fixed.
How has this been tested?
Please describe the tests that you ran to verify your changes.