Feature - config masking

[jsynack]

trafficstars

Implement basic config field masking for the password field.

The goal is to increase the safety of debugging by prevent the logging of the password unless the end-user disables the masking with mask_sensitive: false in the configuration.

This helps to keep debug logs a little cleaner from sensitive information and requiring less sanitization before opening Issues/etc.

This sets the default behavior to true (masking enabled). As debugging is not a feature enabled by default - this should have minimal collateral impact on the installed-base.

This is for Oxidized configuration only - this isn't about the device configurations - that belongs in model-specific areas with remove_secret.

I used a regex to match the fields - it could also be adjusted to an array if that is preference.

[codecov-io]

Codecov Report

Merging #1301 into master will decrease coverage by 0.13%. The diff coverage is 35.71%.

@@            Coverage Diff             @@
##           master    #1301      +/-   ##
==========================================
- Coverage   60.41%   60.28%   -0.14%     
==========================================
  Files          28       28              
  Lines        1349     1357       +8     
==========================================
+ Hits          815      818       +3     
- Misses        534      539       +5

Impacted Files	Coverage Δ
lib/oxidized/config.rb	33.33% <0%> (-2.09%)	:arrow_down:
lib/oxidized/node.rb	66.66% <50%> (-0.23%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6bc4f7e...250a519. Read the comment docs.

[ytti]

No strong opinion here. I wonder if the mask should be hash/dict as it has many keys so instead of mask_x mask.x?

[jsynack]

Greetings @ytti -

I can see it multiple ways as well -

My original thought process was to make it easy to mask more (by using an RE) - but this can also have a negative impact of masking something that is not intended when someone else introduces a new config-key later on. By that time the RE would simply look like a hash/dict anyways of anchored/exact matches. A hash/dict approach is probably safer to prevent false masking and to keep the RE from just getting obnoxiously large/exact.

So it is probably smarter/safer to have an explicit hash/dict to avoid unintentional masking of newly introduced vars.

I also found some similar discussion here regarding the REST API and masking of fields (specifically for oxidized-web): https://github.com/ytti/oxidized/pull/391

If there were attributes that we could apply to a configuration var then there would be a generic way to mask them from logs as well as API returns.

[ytti]

I'm not sure we're talking about the same thing. I'm just thiknin in config it could be

mask:
  a: asdfasdf
  b: 123123
  c: asdasdf

Instead of

mask_a: asdfasdf
mask_b: 123123
mask_c: asdfsdaf

Which you can accomplish just by changing the _ to .

[wk]

Cool feature! It may be better to have this off by default since debug itself is off by default - and having a user turn on debugging just to find out they didn't turn on all of the debugging is an anti-pattern.

Would you consider replacing the configurable mask for a salted, truncated hash of the value? Such as the first 8 characters of sha256(value + 'Oxidized') or similar? This has a few benefits:

• It does not disclose the content or length of the value in plaintext or direct hash form, acting as an effective mask
• It allows a user troubleshooting an issue to gleam some otherwise useful debugging information (for example: "all these nodes were ran with use the same username, but different passwords")

[ytti]

@wk for your purpose the salt can be runtime random, generated each time once, so you wouldn't even know the salt, but would still give benefits you explain.

[wk]

@ytti that's very reasonable. The SecureRandom.hex method from the core securerandom module seems to be particularly suited for this purpose.

[jsynack]

Greetings @wk and @ytti and thank-you for the discussion,

Ironically - my first thought was exactly what you mention @wk - salt/hash and output. This was for the benefit of item 2 - being able to see if something got corrupted or read incorrectly.

I decided to go a little a more simple method of just plain redaction as it seemed like an easier thing to spot in logs and document versus deciding if the output was 'real' or not.

I like the idea of a runtime generated salt and thus zero knowledge of it - the only item I would see would be to explicitly mark the output that it is in this format from what the end-user supplied in to avoid the 'oxidized is reading my password configuration wrong!' type of issues coming in. ;)

The 'active by default' is a byproduct of err'ing on the side of caution when it comes to security issues. And I perhaps just dislike the logging of authentication credentials without a two-step process (not quite two-man rule lol..)

I didn't want end-users to accidentally post a log file in a rush that contained credentials or have to redact it manually. Manual redaction runs the risk of missing one of the entries in a large log file. That would be quite a bit of explaining to do - and (IMO) we should try to make it where someone doesn't risk losing their job by trying to do their job (using oxidized and getting it working).

I agree that it adds 'one more step' in order to do full debugging of everything - but it also seemed like it would be 'obvious to the user' when they saw ******** - that they would understand something was being redacted versus being 'corrupted' (seeing 'random' ASCII chars instead). I may be biased on that being 'obvious' though. Perhaps a log message could assist here as well as one of the first logs on startup to indicate to the user.

[wk]

A clear log early on with content similar to "Oxidized debugging output is configured to obfuscate sensitive output such as authentication credentials. Set 'mask_sensitive: off' in $config to disable'" seems like a reasonable compromise to me.

As for the masked values being obvious, they might very well be set to something along the lines of OBFUSCATED-a0b0c0 or HIDDEN-a0b0c0d0 to give the user a mental hint on what's going on.

[ytti]

As long as you change the mask to mask.parameter so that it becomes

mask:
  parameter: X

In config. I'm cool to pull this, I'm ambivalent on the default. I'd prefer to see hashing instead of star, but won't mandate it either.

[mortzu]

any news on this?