Masking rewrite

[jpcoenen]

Complete rewrite of the masking functionality, with the following goals:

1. Fix output to stdout and stderr getting mixed when written approximately at the same time. (stdout and stderr mixed in secrethub run #196 )
2. Make code more legible, understandable and robust. The original implementation had some different constraints and requirements than what is has evolved to. This has led to a lot of unnecessary complexity, which can be removed by rethinking the design.
3. Reduce the data-dependency on the timing of the masking functionality. The previous implementation had a strong relation between whether input contains secrets and the time consumed by the masker. With the original assumption that only the secret's owner has control over the input of the masker, this was no issue. However, as the masking functionality has broad applicability, it was decided to reduce this data-dependency as much as possible.

[jpcoenen]

Should have said this in the PR description: but for now I'm mainly looking for a high-level review. So: is this new structure we want to go ahead with? Then I'll polish it.

[mackenbach]

Small suggestion: @jpcoenen if you want high level overview, maybe draw a quick diagram of how it works? Even a handdrawn sketch would help.

[jpcoenen]

Will do 👍 That's coming tomorrow.

[jpcoenen]

@SimonBarendse @mackenbach I've done quite a rewrite. It might still look very complex. However, I think that it has been reduced to the inherent complexity of the required behaviour. The signature of every function is as simple as I could get it.

A global overview of how it works (also included a diagram; hope it helps):

Ingesting frames

1. stream.Write() is called whenever the child process writes something to stdout or stderr. Then this happens:
   a. IndexedBuffer.Write(): we store the incoming bytes (a frame) in a buffer
   b. multipleMathcer.Write(): we directly check for any secrets.
   b. stream.addMatch(): If we find any secrets, we store them in a map with the index of the match (which is incrementing and unqiue over the lifetime of the masker) and the match's length.
   c. Masker.registerFrame(): We also set a timer to flush all the bytes of this frame after BufferDelay has passed.

Flushing frames

Run() (executed in a goroutine) continuously waits for timers to expire. if this happens, it calls stream.flush(), which writes the frame to the destination io.Writer, while masking any previously found secrets.

[mackenbach]

Very comprehensive rewrite/refactor. It looks like you were able to reuse quite a lot of the old logic that was sound and simplified the rest. I have quite a few comments, mostly architectural and naming/documentation.

Note: Last time we touched this functionality, I also made remarks as to the readability and complexity of the code and we then decided to improve that later... which became never. So I hope we can prioritize readability and simplicity now to avoid having to do the same dance a third time.

On the requirements side, I have a few questions that I think are good to tick off explicitly:

• Is the secret masking constant-time? If so, I feel we should mention that somewhere.
• Is the masking still a best-effort attempt? If so, I feel we should mention that somewhere.
• Does it work nicely with the TTY cases where output is written line-by line?
• How does it handle multi-line secrets? And if multi-line secrets are written line by line, how does that affect the masking behavior?
• Does it work nicely with (non-TTY) cases where bytes are not buffered before being written to our writer? If I understand your code correctly, that would mean every frame is one byte long?
• Does it not write to both (stderr and stdout) output streams at the same time (at least in the TTY cases)? In other words, does it keep both output streams from interfering?
• Finally, and this is one that didn't come up in the requirements discussion but I realized while reviewing (see also comments in the review): can this run without errors for a long time and with a lot of output being processed over time (e.g. a use case where a server is wrapped with secrethub run and runs for a year, writing log lines to stdout)?

Overall, very well done. I'm happy we are able to (finally) fix this and in a pretty elegant way too.

[mackenbach]

Is it Run() or Start()? If Start() makes more sense, then I'd expect there to be a Stop() function too probably. Now the stopping behavior seems to be controlled by the flushAllChan, which is private, but is documented in the public docs here.

[mackenbach]

Also, do 'streams' still accept write calls after Flush has been called? And is that desired (and documented)?

[jpcoenen]

Good suggestion.

Also, do 'streams' still accept write calls after Flush has been called? And is that desired (and documented)?

// This should be run after all input has been written to the io.Writers of the streams.

I'll see whether I can make this a little more explicit.

[jpcoenen]

You will get a panic if you write to a masked io.Writer after calling Close(). I've documented that behaviour. Catching without a panic requires some messy logic because streams would have to know whether the masker has stopped or not.

[mackenbach]

Should we catch that error? You could wrap the streams with a very simple io.Writer interface that does exactly that one check. Can be a simple writeFunc or something.

[jpcoenen]

Hmm, interesting approach. Will look into it.

[jpcoenen]

Getting this solid is not as easy as I thought. For example, what happens if your in a write when you stop the masker? At the moment the behaviour is undefined because you're using the masker wrong. If we start guarding for this, I feel like we should do it properly, but that again adds some complexity. Is that what we want?

[mackenbach]

Yes, there's definitely a tradeoff here. Consider adding to the docs that writing to any of the writers (/streams?) after calling Masker.Close() causes a panic. I think that should cover it well enough for now and we can add complexity when we find a valid use case for it.

[jpcoenen]

That's already there https://github.com/secrethub/secrethub-cli/pull/267/files#diff-331cd1ff2b2231141462b60c73817d48R108

[SimonBarendse]

Are you still looking for only a high level review?

Based on your diagram, the high level implementation looks good! Looks like the two important jobs, masking and buffering/timeouts are nicely separated.

Is there a need for the bytes to go through the masking first and then in the buffer? Seems like going from the stdout write directly to the buffer could simplify it a bit more, because the bytes don't have to pass through the "stream" (masking engine) one more time. But, while this might seem complex in the diagram it might work out pretty smooth in the implementation?

[jpcoenen]

Are you still looking for only a high level review?

Always welcome, but I am pretty convinced now that this architecture is more solid than the previous one.

Is there a need for the bytes to go through the masking first and then in the buffer? Seems like going from the stdout write directly to the buffer could simplify it a bit more, because the bytes don't have to pass through the "stream" (masking engine) one more time. But, while this might seem complex in the diagram it might work out pretty smooth in the implementation?

I don't really get what you mean here. If you think it's important, let's discuss it face-to-face.

[florisvdg]

@SimonBarendse should I focus the review on the code or more on the functionality?

[mackenbach]

@jpcoenen one question that just hit me: what happens if you have a secret to match that is max size (~512KB if I recall correctly)? Would that create a lot of detectors? Or does it have to do with the starting sequence of the secret?

[jpcoenen]

@jpcoenen one question that just hit me: what happens if you have a secret to match that is max size (~512KB if I recall correctly)? Would that create a lot of detectors? Or does it have to do with the starting sequence of the secret?

It only checks for repetitions of the start of the secret. So the size should not matter, only the degree of repetition. If it starts with

aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

That does indeed create >300 detectors. If it starts with:

caaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

It does with 1 detector.

Would the first scenario be something we want to guard against?

[mackenbach]

Great explanation, thanks!

Would the first scenario be something we want to guard against?

No I think it's fine. It would be a very weird edge case and caused by weird behavior on the part of the user. Although we could implement a limit, e.g. max 100 repeating first characters, and document that if the limit is exceeded it isn't guaranteed to mask all (partial) matches?

It's a tiny edge case though. We could also implement it when someone runs into an issue. Right now, the way I get it is that it still masks fine, just the memory (and CPU?) footprint increase hugely, right? That's acceptable for a very weird edge case for now I'd say.

[jpcoenen]

Great explanation, thanks!

A good question deserves a good answer.

It's a tiny edge case though. We could also implement it when someone runs into an issue. Right now, the way I get it is that it still masks fine, just the memory (and CPU?) footprint increase hugely, right? That's acceptable for a very weird edge case for now I'd say.

Yep. Probably becomes unusable. I agree with your conclusion.

[jpcoenen]

Updated the diagram:

[mackenbach]

Solid, with the new diagram you really see how a very complex technical problem can be solved elegantly with no more than necessary complexity. Very good stuff guys.

One tiny typo: I think it's called go masker.Start() now right?

[mackenbach]

Okay stupid question that might fuck up your day: how does the masker.Masker depend on the encoding of the sequences [][]byte passed to the constructor AND the encoding of the streams (stdout and stderr)?

For instance, we've encountered a few times UTF16 encoding on Windows. Now I don't know what encoding windows uses for stdout and stderr, but I think it's worth checking for a second:

1. Is there a real scenario where encoding is different than we're used to?
2. What would be the impact if that occurred?
3. Do we want to guard against that, either with code or with documentation?

[jpcoenen]

Okay stupid question that might fuck up your day: how does the masker.Masker depend on the encoding of the sequences [][]byte passed to the constructor AND the encoding of the streams (stdout and stderr)?

For instance, we've encountered a few times UTF16 encoding on Windows. Now I don't know what encoding windows uses for stdout and stderr, but I think it's worth checking for a second:

1. Is there a real scenario where encoding is different than we're used to?

2. What would be the impact if that occurred?

3. Do we want to guard against that, either with code or with documentation?

That's a good point. At the moment, if something is written to stdout in UTF-16, it is not masked. We could cover this by introducing a --masking-encoding flag. But I feel that is overkill for now. You are right that we encountered problems with this on Windows, but I don't think that's relevant in this case. The program itself determines what encoding is used in the output. All programs/languages I tested use ASCII or UTF-8, which both seem to work fine.

Unicode characters themselves are supported. ⓗⓔⓛⓛⓞ ⓣⓗⓔⓡⓔ gets masked normally.

For now, we could ask to the docs that only UTF-8 output gets masked.

[mackenbach]

Awesome, good balanced response.

Unicode characters themselves are supported. ⓗⓔⓛⓛⓞ ⓣⓗⓔⓡⓔ gets masked normally.

Do we want to add a test for good measure?

For now, we could ask to the docs that only UTF-8 output gets masked.

Yes let's do that.

[SimonBarendse]

@SimonBarendse should I focus the review on the code or more on the functionality?

@florisvdg As discussed in person during standup, I asked mainly for shared understanding and code ownership to ensure we can maintain this properly and smoothly. This is a complex piece of code, which I think deserves the attention of all of us. Any questions you might have can be reflected in the documentation, to further enhance maintainability.

This is good to go 🚀, but let's make sure to go over this soon @florisvdg , while it's still fresh in our heads.

[SimonBarendse]

Before shipping, could you still address this @jpcoenen ?

Unicode characters themselves are supported. ⓗⓔⓛⓛⓞ ⓣⓗⓔⓡⓔ gets masked normally.

Do we want to add a test for good measure?

[mackenbach]

PR descriptions get committed, right? If so, please edit it so it's a bit more helpful for later.

[SimonBarendse]

[jpcoenen]

@SimonBarendse this means you tested it?

[SimonBarendse]

@SimonBarendse this means you tested it?

Yes