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

ARTEMIS-1740: Add support for regex based certificate authentication #2011

Closed
wants to merge 2 commits into from
Closed

ARTEMIS-1740: Add support for regex based certificate authentication #2011

wants to merge 2 commits into from

Conversation

LionelCons
Copy link
Contributor

This adds the possibility to have an optional properties file containing regular expressions to match against the DN.

@clebertsuconic
Copy link
Contributor

@jbertram can you look into this one? you have more miles on security than I do. :)

@jbertram
Copy link
Contributor

jbertram commented Apr 11, 2018

This looks good except for a few things:

  • I would rather not have an additional file for the regex properties. I think it would be better to instead denote the regex in the normal user properties file using the traditional forward slashes (as described in the JIRA).
  • There's no documentation. The new functionality should be documented in docs/user-manual/en/security.md.
  • I'd like to see an integration test added to org.apache.activemq.artemis.tests.integration.security.SecurityTest. There's several tests in there already which test cert-based login.

@@ -95,6 +99,21 @@ public synchronized ReloadableProperties obtained() {
return invertedValueProps;
}

public synchronized Map<String, Pattern> regexpPropertiesMap() {
Copy link
Contributor

@franz1981 franz1981 Apr 12, 2018

Choose a reason for hiding this comment

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

You could use a Suppliers::memoize to allow a thread-safe lazy initialization without having that method synchronized even when you just need to get regexpProps.
But is is needed?

String dn = getDistinguishedName(certs);
String name = usersByDn.get(dn);
if (name == null && regexpByUser != null) {
name = getUserByRegexp(dn);
Copy link
Contributor

Choose a reason for hiding this comment

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

getUserByRegexp is synchronized but there are uses of usersByDn and regexpByUser, like these ones, that are not: what is the reason to have String getUserByRegexp(String dn) synchronized?

 - not using a separate file for regexps anymore
 - added negative caching
 - added more tests
 - added documentation
@LionelCons
Copy link
Contributor Author

The intent of synchronized is to protect the modification of usersByDn.

@franz1981
Copy link
Contributor

franz1981 commented Apr 12, 2018

@LionelCons

And it makes sense, but there are parts like:
String name = usersByDn.get(dn);
that are not synchronized, hence it won't be thread-safe.
My advice is to use a lazy initialization through a Suppliers::memoize or similar construct and always use Supplier::get to access the variable: that would allow to have always a thread-safe access to an unmodifiable resource, lazy initialized.

@LionelCons
Copy link
Contributor Author

@franz1981 your comment goes beyond my modifications since the existing code already uses the get method (in getUserNameForCertificates and getUserRoles, at least).


return usersByDn.get(getDistinguishedName(certs));
String dn = getDistinguishedName(certs);
String name = usersByDn.get(dn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hence this userByDb not synchronized access no longer exists in the new commit?
With github and multiple commits is not simple to do reviews :P

@LionelCons
Copy link
Contributor Author

The final code can be seen here.

Just like in the original code, both getUserNameForCertificates and getUserRoles still use get.

@jbertram
Copy link
Contributor

Thanks for the changes, @LionelCons. Can you squash your commits?

@LionelCons
Copy link
Contributor Author

LionelCons commented Apr 12, 2018

@jbertram can you squash the commits when accepting the merge?

@clebertsuconic
Copy link
Contributor

@LionelCons I can.. if this is ready to be merged.

@clebertsuconic
Copy link
Contributor

done

@asfgit asfgit closed this in 5535af1 Apr 12, 2018
@LionelCons LionelCons deleted the artemis_1740 branch May 8, 2018 08:08
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.

4 participants