-
Notifications
You must be signed in to change notification settings - Fork 31
Redact JWT signature blocks in logs #2836
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
Conversation
jhiemstrawisc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to #2828, I think the real bug here is that we've identified something that should happen for both Client/Server initialization, but we still perform that initialization separately in each component, leaving room for skew. It seems like initFilterLogging really belongs somewhere central like InitConfigInternal().
@jhiemstrawisc You might believe in this. It might be a good idea. Perhaps a great one, even. Heavens knows I gave it a try. But I'm too naïve to make it work. Empirically, FlushLogs must be called before initFilterLogging (or cannot be called after it?) in order for the regex filter to actually be applied to the logs' output. There must be something I'm not grokking that explains why the ordering of these two functions matters here. |
382a44f to
d051dfd
Compare
|
In the meantime, I've rebased this PR on the current Assuming you're starting from a completely fresh setup (in particular, no browser cookies associated with your origin), the minimal test for this functionality is something like:
In the client's output, check for lines like the following (watch as I nonchalantly copy my output verbatim, which is the entire point of this PR): In the server's logs, check for lines like the following: |
|
Another useful test here is to try out the functionality from #2897, e.g., |
|
@brianaydemir I think I got to the bottom of at least one of these GHA failures... The crux of the failing CI test in the logs is here. The lines following that, up until the Why, you might ask? The Thoughts? |
d051dfd to
a7d6799
Compare
|
So, it looks like we want the client to print out version information as a diagnostic when debug logging is enabled. Reasonable enough. I'm not sure I entirely understand how This instance of printing out the client's version is for diagnostic purposes; the output should go to the same location as other diagnostic output (logs), and not clutter |
For debugging purposes, the existence of the refresh token is worth knowing, as are the contents of the access token. Everything else has not proven useful in my experience.
I've gone back and forth on whether to include this, but now that I understand the redaction mechanism for log lines, I'm more comfortable including this bit of useful information.
a7d6799 to
7553d6f
Compare
jhiemstrawisc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I had a quick chat with @patrickbrophy about the choice to send version info to the log instead of stdout (I recall Patrick working on that last, haven't checked git blame), and he said the change seems reasonable.
I built locally and confirmed version info is logged and that my token signature is being redacted (per the instructions) in the debug output.
Pelican was already redacting JWT signature blocks in server logs. This PR adds the same functionality to the client.
In addition, the client no longer logs the contents of the refresh token it receives from the device code flow. (As far as I'm aware, the refresh token can be an arbitrary string, so we have to parse through the response from the server.)