Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add support for artifactory identity tokens #115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dpittner
Copy link
Member

@dpittner dpittner commented Jan 5, 2023

What: when using Artifactory identity tokens (https://www.jfrog.com/confluence/display/JFROG/Access+Tokens) which are a recent feature you've the option to make them expiring. So it's desirable to use them instead of pro-active rotation.

However the token format is different from what's currently detected by the plugin, so this PR aims to extend the plugin with the ability to detect identity tokens of artifactory.

@dpittner dpittner marked this pull request as ready for review January 5, 2023 08:22
Copy link
Member

@tefiggins tefiggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR build step is failing ... please debug ... python code is sensitive to spacing .. maybe it is an issue indentation of your code change?

@@ -15,6 +15,8 @@ class ArtifactoryDetector(RegexBasedDetector):
re.compile(r'(?:(?<==|:|")|(?<=\s)|(?<=^))AKC[a-zA-Z0-9]{10,}'), # api token
# artifactory encrypted passwords begin with AP[A-Z]
re.compile(r'(?:(?<==|:|")|(?<=\s)|(?<=^))AP[\dABCDEF][a-zA-Z0-9]{8,}'), # password
# artifactory identity tokens are different (base64 encoded reftkn:) and 64 chars
re.compile(r'(?:(?<==|:|")|(?<=\s)|(?<=^))cmVmdGtuOg{54,54}'), #identity token
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple questions: Where did you get the regex that you included in this PR? Also do all artifactory tokens have "cmVmdGtuOg" literal string?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IF reading this right:

line 15: says: start with AKC then [any lower or upper case alphabet or number char ] and then 10 or more of the same preceding token (ie lower or upper alphabet or number)
line 17: says: start with AP then [any upper case alphabet A-F or number char ] and then [any lower or upper case alphabet or number char ] 8 or more of the same preceding token (ie lower or upper alphabet or number)
line 19 (your new regex) say: starts with c then m then V then m then d then G then t then u the 0 the g then exactly 54 more characters of preceding (ie g)

1st, correct me if I am understanding this wrong. But if I am correct, then this will not work as a general enough regex and it would only make one very specific value.

Copy link
Member

@tefiggins tefiggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not see any additions under the test directory for unit-testing your new toke addiction.

@d3rnn d3rnn mentioned this pull request Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants