Handle Dynatrace API Token in the sanitizer #974
Conversation
There was a problem hiding this comment.
I think this is a good start, but it's really only addressing the specific case of dynatrace, we should probably make this a general solution. I can imagine that there are other things that should be sanitized from download URLs.
Can you do something like this?
- Make a list of query params to sanitize. The list can be strings or regex patterns.
- For each item in the list, go through the query params, and if the param key matches (case insensitive), then set the value to
****. I don't think we need to care about the value. If the query param key matches, then we just overwrite it. - Add some unit tests to show this working.
Some keys for the list, just trying to think of possibly sensitive query params:
username
password
token
api
key
secret
access[-_]token
secret[-_]token
api[-_]key
api[-_]token
auth[entication]*
cred[s]*
and feel free to add anything else that comes to mind.
|
I have also already thought of implementing a general solution. However, this is a problem for which we need a solution quickly. This was the least invasive fix from our point of view. |
|
To speed things up a little bit I would also appreciate if this could be implemented in two phases so that we get the Dynatrace API token masking with this PR and a more general solution with a follow-up PR. |
|
My vote would be for a proper fix, I don't think it's a huge request. That would also add in a solution that's actually generic, right now, this PR is modifying core parts of the buildpack with DT-specific logic. |
|
Or this PR is merged, satisfying a shared customer of VMware and Dynatrace. This way SAP can continue with their project. In return we will commit to adding this sanitizer in our own time, without throwing our current planning overboard. |
|
Hi @dmikusa, |
lib/java_buildpack/util/sanitizer.rb
Outdated
| if(match[0] == "Api-Token" && value =~ /dt\w*/) | ||
| params[key] = value.gsub(/(dt\w*\.\w*)\.\w*/, '\1.REDACTED') |
There was a problem hiding this comment.
What is the need for this inner if block? Why can't we just have everything be masked in the same way?
There was a problem hiding this comment.
Thanks for your review. The if is needed because we only want to mask parts of the Dynatrace API Token. The Tokens have a public and a private part. Masking only the private part helps in identifying which token was used for staging the app by looking at the logs.
There was a problem hiding this comment.
Just one note from my side. My concern is that if we manipulate the URI we might end up with similar Hashes for different files, possibly breaking other stuff in the Buildpack. We are touching the Buildpack Code at a very low level here. This Buildpack is a very complex piece of software and we should be aware of possible side effects.
The sanitize_uri function is used in different places, mainly for Logging URL's and generating a filename for the cached file artifacts. (File, Etag, Last-Modified)
The Download Cache itself is used for a lot of things. Main Use Case is for downloading Stuff directly by using the download function in the Base Component but also in some other components like External Config or the Repository Index.
But only someone with a lot of knowledge about all the mechanisms in the Buildpack can judge possible side effects. I can't.
The pull request is also missing the tests.
There was a problem hiding this comment.
Hi @dmikusa,
I've updated the code to fulfill the RuboCop recommendations and also added a test. Please have another look. Thanks in advance.
There was a problem hiding this comment.
Thanks for this PR & for bearing with me through the process.
I'm not a huge fan of having the special case part in there, but mostly because I think it could be potentially difficult to handle. If there's one special case, then that invites the potential for others. We can cross that bridge if it becomes an issue though.
The sanitize_uri function is used in different places, mainly for Logging URL's and generating a filename for the cached file artifacts. (File, Etag, Last-Modified)
This is a good point. If it's caching a URL with the masked value, that would be less unique and could introduce collisions (i.e. the masked value changes, but it's not reflected in the URL for caching purposes).
Since the buildpack cache is scoped to your application, I'm not sure this would be a huge issue. Taking Dynatrace for example. The user would need to perform a build with one key and then switch the key and rebuild. In that case, I think you'd get a collision. It's hard to say if that would be a problem. In most cases where everyone gets the same set of binaries, it wouldn't be. If the binary being fetched was customized and unique to the particular URL then that could be an issue.
I could see this being handled in two ways.
- We could adjust the caching to pull the real URL, not the masked URL, that way we have the full and unique URL for caching purposes. The trick with this approach is that we'd need to be really careful to not have the actual URL saved somewhere in the cache as that would be another form of information leak.
- Since most dependencies are not impacted by this, as they are not unique per app/user/token, we could handle this for the particular framework. Essentially, the framework having domain knowledge would know if the dependency might be susceptible to this problem. It could then clear the cache when it detects a change to the secret/token/password/etc.
At any rate, I'm not sure if we need to deal with this now or if we can wait and follow up with another PR. I'd defer to you on that, given you know how this might impact Dynatrace users. What do you think?
There was a problem hiding this comment.
@dmikusa
I agree that the special handling of the DT Token in that place is not a good idea. I suggest removing that part again. Then we have at least the problem with logging secrets solved for now.
|
@dmikusa Is there any new development here? We need to get this merged for a customer asap! |
|
@meibensteiner Hi, posted some thoughts. Sorry for the delay. I don't work full time on this anymore, just spare time/OSS capacity. @pivotal-david-osullivan - FYI for integration testing/release and also for a second opinion. |
|
@dmikusa Hi, I've removed the special handling for the Dynatrace token. Please have another look. |
|
@pivotal-david-osullivan Or maybe you could have a look at it? |
|
Dear @dmikusa, @pivotal-david-osullivan, Reviewing the PR should probably not take longer than 10 minutes. Thanks for your support! |
|
Yes, we'll take another look at this ASAP, thanks! |
Masking the private part of the API Token
Fix for Issue #967