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

Support Gitlab variable masking #19

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eds-collabora
Copy link
Collaborator

@eds-collabora eds-collabora commented Nov 25, 2022

It's obviously better for this single crate to support variable masking, rather than replicating support in each client.

This PR automatically masks the trace sent to Gitlab, and furthermore permits crates to upload artifacts as masked if they need to (e.g. logs).

This PR is draft pending:

  • Publishing of required update to masking crate.
  • Final testing.

This brings in a dependency on the mask crate. The code to support
masking artifacts will come later. This only masks the logs passed to
Gitlab.
This is only a basic test that the masking is in fact being applied to
variables marked as masked. This is not a test of the functionality of
the masking crate, only of its integration into this crate.
Although we already automatically mask the trace sent to Gitlab, some
runners will produce additional log files or metadata, which must also
be masked to prevent secrets from being revealed in the artifact data.

This permits client crates to identify a file they wish to upload as
additionally requiring masking.

Note that the version bump in the masker crate is because we now need
to be able to clone the masker for each Uploader to use.
@eds-collabora eds-collabora marked this pull request as ready for review November 25, 2022 16:37
@sjoerdsimons
Copy link
Collaborator

This does the masking at the point where traces get send to gitlab which is obviously good; However this still means if the runner runs at trace debug level these potentailly sensitive variables end up in the system log; I wonder if it would be worth it pushing things in a higher logging/filtering layer?

@eds-collabora
Copy link
Collaborator Author

This does the masking at the point where traces get send to gitlab which is obviously good; However this still means if the runner runs at trace debug level these potentailly sensitive variables end up in the system log; I wonder if it would be worth it pushing things in a higher logging/filtering layer?

I've been investigating this. It's a bit tricky because there are (as far as I can see) two more levels we could do this at:

  • Universally, by installing something into the tracing machinery, so that everything that gets logged gets masked. This, at worst, requires each client crate to do it once correctly, and perhaps we could bake in a recipe to obtain the tracing stack in this crate.
  • Partially, by changing the log function (e.g. outputln!) to mask as things enter tracing. The problem here is that all the other mechanisms to generate data for tracing still exist and now carry the risk that (when the mechanism by which masking is being accomplished is obscured or forgotten) might be used without the necessary filtering.

I strongly favoured injecting masking into the tracing machinery, but my investigations here have shown this to be quite difficult. While it's possible to define a tracing-transform crate that optionally replaces the emitted tracing events with modified ones, actually constructing the modified tracing events is really difficult: lots of things are held as references (possibly static references), and the constructors for things like ValueSet are private.

Tentatively, at this point, I think the way forward in this direction would be to try to create a transform function within tracing itself and see if that addition will fly, although I'm open to suggestions for the best way to go here.

I don't think just changing outputln! or similar is ideal: although it is easy enough to do, it gives a false impression of safety, in precisely the sort of place we don't want that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants