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

Configurable masking and hiding for the env endpoint #6711

Merged
merged 12 commits into from
Jan 25, 2022

Conversation

timyates
Copy link
Member

@timyates timyates commented Jan 5, 2022

  • Changes the env endpoint to be disabled by default (requires user to enable in config)
  • If enabled, all env endpoint values are masked with *****
  • Allow user to add a bean implementing EnvironmentEndpointFilter which:
    • allows legacy operation to be allowed so it behaves as before
    • can be set to mask or reveal all
    • allows patterns for masking to be defined if reveal all is set
    • allows patterns for plain-text values if mask all is set

The call to the filter also contains the Principal if there is one, so the user can choose
to alter visibility of items based on the currently logged in user

- Changes the env endpoint to be disabled by default (requires user to enable in config)
- If enabled, all env endpoint values are masked with *****
- Allow user to add a bean implementing EnvironmentEndpointFilter which:
  - allows legacy operation to be allowed so it behaves as before
  - can be set to mask or reveal all
  - allows patterns for masking to be defined if reveal all is set
  - allows patterns for plain-text values if mask all is set

The call to the filter also contains the Principal if there is one, so the user can choose
to alter visibility of items based on the currently logged in user
@CLAassistant
Copy link

CLAassistant commented Jan 5, 2022

CLA assistant check
All committers have signed the CLA.

@timyates timyates changed the title New version is 3.3.0-SNAPSHOT Configurable masking and hiding for the env endpoint Jan 5, 2022
@timyates timyates linked an issue Jan 5, 2022 that may be closed by this pull request
@timyates timyates self-assigned this Jan 5, 2022
@timyates timyates added this to the 3.3.0 milestone Jan 5, 2022
For binary compatibility.  And annotate new constructor with Inject
so it is used
@timyates
Copy link
Member Author

timyates commented Jan 7, 2022

@graemerocher Hope I addressed all your feedback 🤞

@graemerocher
Copy link
Contributor

@timyates from my side yeah, @jameskleeh needs to review next

Copy link
Contributor

@jameskleeh jameskleeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is missing:

  • the ability to mask all properties except a set a patterns
  • the ability to mask all properties except a set of static keys
  • the ability to mask a set of static keys

* @return itself for chaining calls.
*/
public @NotNull EnvironmentFilterSpecification maskPatterns(Pattern... propertySourceNameMask) {
maskedPatterns.addAll(Arrays.asList(propertySourceNameMask));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have no effect without allMasked = false; so that should probably be set here

Copy link
Member Author

@timyates timyates Jan 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If allMasked = true, then this sets the patterns that are allowed to be in the clear

https://github.com/micronaut-projects/micronaut-core/pull/6711/files#diff-4e7e5d0b9696543f54847a8d042130fc33e81143e23d6c414504d106ec0fa4c7R15

maskPatterns is probably a rubbish name... But toggleMask was wrong too, as it's not a toggle, it's additive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maskPatterns should define the patterns that are masked, thus only making it useful if not all keys are masked

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a separate list for unmasked patterns, that only come into play if allMasked is true?

Or we could name it "excludedPatterns" which are excluded from whichever mode you're in... And change the method to exclude() ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds OK to me. Need to support static keys as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the ability to supply predicates. Everything could be converted to predicates

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to using Predicate<String> for all exclusions

To avoid unchecked generic array creation for varargs parameter warnings in user code (due to the Predicate<String> varargs), introduced EnvironmentFilterNamePredicate as a typealias

Also added a factory method to generate a predicate from a pattern and optional flags

Comment on lines 115 to 121
public static EnvironmentFilterNamePredicate regularExpressionPredicate(String pattern) {
return regularExpressionPredicate(pattern, 0);
}

public static EnvironmentFilterNamePredicate regularExpressionPredicate(String pattern, int flags) {
return s -> Pattern.compile(pattern, flags).matcher(s).matches();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These appear now to be part of the public API? if it is the case they need javadoc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Would you say they were necessary? I added them to make it simpler to create the Predicates, but can see that they may add noise 🤔

@timyates
Copy link
Member Author

Also, having the EnvironmentFilterNamePredicate type-alias as an inner-class makes the javadoc quite long 😐 Should it be made top-level? Or does someone know of a better way around the unchecked generic array creation for varargs parameter warning?

@timyates
Copy link
Member Author

After a sync up with James, we have a better API... Incoming asap

@jameskleeh
Copy link
Contributor

@graemerocher Can you please give this a final look?

@jameskleeh jameskleeh merged commit 7c6149c into 3.3.x Jan 25, 2022
@jameskleeh jameskleeh deleted the timyates/environment-endpoint-filtering branch January 25, 2022 15:02
yawkat pushed a commit that referenced this pull request Jun 24, 2022
* Configurable masking and hiding for the env endpoint

- Changes the env endpoint to be disabled by default (requires user to enable in config)
- If enabled, all env endpoint values are masked with *****
- Allow user to add a bean implementing EnvironmentEndpointFilter which:
  - allows legacy operation to be allowed so it behaves as before
  - can be set to mask or reveal all
  - allows patterns for masking to be defined if reveal all is set
  - allows patterns for plain-text values if mask all is set

The call to the filter also contains the Principal if there is one, so the user can choose
to alter visibility of items based on the currently logged in user

* Address first round of feedback

* Fix tests

* Fix since tag

* Add single arg constructor

For binary compatibility.  And annotate new constructor with Inject
so it is used

* Switch to Predicate<String> in place of Patterns

* Checkstyle fixes

* Move env endpoint into disabled section of doc

* Add deprecated methods and JavaDoc

* Remove principal and simplify API

* Cleanup

Co-authored-by: jameskleeh <james.kleeh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Environment Endpoint security issue
5 participants