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 role mapping for Client certificates #23683

Closed
ruromero opened this issue Feb 14, 2022 · 22 comments · Fixed by #37269
Closed

Support role mapping for Client certificates #23683

ruromero opened this issue Feb 14, 2022 · 22 comments · Fixed by #37269
Assignees
Labels
area/security kind/enhancement New feature or request
Milestone

Comments

@ruromero
Copy link
Contributor

Description

When using Mutual TLS Authentication it is not possible to add roles to identities without a custom SecurityIdentityAugmentor it would be great to be able to extract the roles from a file or an external source like a database or LDAP.

That means that when configuring MTLS I cannot have RBAC without implementing it myself.

Implementation ideas

Would it be enough to just provide a default SecurityIdentityAugmentor that is self-configured based on the quarkus.security.users.file.roles property or quarkus.security.jdbc.roles.query (to give some ideas) ?

This is an adapted working example taken from the guides.

@ApplicationScoped
public class RolesAugmentor implements SecurityIdentityAugmentor {

    private static final Logger LOGGER = LoggerFactory.getLogger(RolesAugmentor.class);

    @ConfigProperty(name = "quarkus.security.users.file.roles")
    Optional<String> rolesPath;

    Map<String, Set<String>> roles = new HashMap<>();

    @PostConstruct
    void loadRoles() {
        if (!rolesPath.isEmpty()) {
            Properties rolesProps = new Properties();
            try {
                rolesProps.load(this.getClass().getClassLoader().getResourceAsStream(rolesPath.get()));
                rolesProps.entrySet().forEach(e -> {
                    LOGGER.debug("Added role mapping for {}:{}", e.getKey(), e.getValue());
                    roles.put((String) e.getKey(), parseRoles((String) e.getValue()));
                });
            } catch (Exception e) {
                LOGGER.warn("Unable to read roles mappings from {}", rolesPath, e);
            }
        }
    }

    private Set<String> parseRoles(String value) {
        Set<String> roles = new HashSet<>();
        for (String s : value.split(",")) {
            roles.add(s.trim());
        }
        return roles;
    }

    @Override
    public Uni<SecurityIdentity> augment(SecurityIdentity securityIdentity, AuthenticationRequestContext authenticationRequestContext) {
        return Uni.createFrom().item(build(securityIdentity));
    }

    private Supplier<SecurityIdentity> build(SecurityIdentity securityIdentity) {
        if (securityIdentity.isAnonymous()) {
            return () -> securityIdentity;
        }
        QuarkusSecurityIdentity.Builder builder = QuarkusSecurityIdentity.builder(securityIdentity);
        CertificateCredential certificate = securityIdentity.getCredential(CertificateCredential.class);
        if (certificate != null) {
            builder.addRoles(extractRoles(certificate.getCertificate()));
        }
        return builder::build;
    }

    private Set<String> extractRoles(X509Certificate certificate) {
        if(certificate == null
                || certificate.getSubjectX500Principal() == null
                || certificate.getSubjectX500Principal().getName() == null) {
            return Collections.emptySet();
        }
        Set<String> matchedRoles = roles.get(certificate.getSubjectX500Principal().getName());
        if(matchedRoles == null) {
            return Collections.emptySet();
        }
        return matchedRoles;
    }

}
@sberyozkin
Copy link
Member

Also CC @stuartwdouglas

@sberyozkin
Copy link
Member

Hi @ruromero I'm not sure this enhancement should be in the form of the security augmentor at the MTLS mechanism level, as it can have ordering issues (the custom ones may not see this augmentation), and I'm also not sure that we need to start supporting reading from the files - as again - someone will want that be in YAML, someone in the simple properties format.

IMHO as far as MTLS authentication mechanism is concerned, it should check a Map property in its own namespace, http.ssl.client-auth=none/request/required/ is a build-time property in HttpBuildTimeConfig. I believe we should introduce HttpConfig in vertx-http/runtime and start there with a http.ssl.client-auth.roles map property which MTLSAuthentocationMechanism will propagate with CertificateCredential and X509IdentityProvider will add the roles for a given subject to SecurityIdentity.
Stuart, Clement, others may have different opinions, this is how I'd consider approaching it. It can make sense to start from a draft PR if you agree...

Thanks

@ruromero
Copy link
Contributor Author

I think a different property makes total sense but I think it'll be enough to treat it the same way as quarkus.security.users.file.roles i.e. a properties file.
Let's wait for other comments and as this is not urgent I can own it once there's agreement, if it is fine for you.

@sberyozkin
Copy link
Member

sberyozkin commented Feb 15, 2022

@ruromero I see, I forgot that quarkus-elytron-security-properties-file supports both embedded.roles and file.roles, so I agree, lets make it consistent, but IMHO that should apply to both options, i.e, we should let users configure the roles mapping directly in application.properties (equivalent to quarkus.security.users.embedded.roles) and in the file (equivalent to quarkus.security.users.file.roles), so that the users don't have to start creating extra files unless they need or prefer it.

It would be good to reuse quarkus.security.users.embedded.roles, quarkus.security.users.file.roles but there are 2 problems here: 1) they are part of the quarkus-elytron-security-properties-file extension which the MTLS does not necessarily need, and 2) the use case where MTLS (with a specific client computer) is enforced but also Basic Authentication is required - so it is better to avoid reusing these properties IMHO.

So the proposal is to have
http.ssl.client-auth-roles Map property to let user do it directly in application.properties(http.ssl.client-auth.roles does not work as http.ssl.client-auth is already a String property) and http.ssl.client-auth-file-roles String property pointing to a file.

What do you think ?

@ruromero
Copy link
Contributor Author

ruromero commented Jun 9, 2022

Sorry I forgot to reply.

The proposal sounds great to me. Thanks!

@jochenr
Copy link

jochenr commented Sep 14, 2023

Hi,
any news here?
Is there any possibility to map the client certificates to roles without a custom implementation?
Thank you!

@sberyozkin sberyozkin self-assigned this Nov 3, 2023
@sberyozkin
Copy link
Member

sberyozkin commented Nov 17, 2023

@jochenr Hi, I'll be looking into it but I can't prioritize just yet. In meantime, can you please provide some examples of how you map things currently ? Do you map complete CA names to one or more roles ?

@jochenr
Copy link

jochenr commented Nov 20, 2023

Hi @sberyozkin,

since we migrate application from (Fuse on) JBoss EAP, it would be perfect if we have the same features as the elytron subsystem.

We don't use the complete CA name as key in the properties file. We use an elytron "x500-attribute-principal-decoder" in the mapper section to cut the name out of the complete CA name like this:

<subsystem xmlns="urn:wildfly:elytron:13.0" final-providers="combined-providers" disallowed-providers="OracleUcrypto">
   [...]
	<security-domains>
		[...]
		<security-domain name="client-cert" default-realm="clientCertRealm" permission-mapper="default-permission-mapper" principal-decoder="CNDecoder">
			<realm name="clientCertRealm" role-decoder="groups-to-roles"/>
		</security-domain>
		[...]
	</security-domains>
	<security-realms>
		[...]
		<properties-realm name="keyStoreAuthorizationRealm">
			<users-properties path="application-users.properties" relative-to="jboss.server.config.dir" digest-realm-name="ApplicationRealm"/>
			<groups-properties path="${jboss.home.dir}/modules/de/xxx/conf/main/properties/certificate-trusted-clients-app-roles.properties"/>
		</properties-realm>
		[...]
	</security-realms>
	[...]
	<mappers>
		[...]
		<x500-attribute-principal-decoder name="CNDecoder" attribute-name="CN" maximum-segments="1" convert="true"/>
	   [...]
	</mappers>
   [...]
</subsystem>

And the we have the CN as key and the values are a list of comma separarted roles

cnname=role1,role2,role3

@sberyozkin
Copy link
Member

@jochenr Thanks, I can prototype something simple and will open a draft PR

@sberyozkin
Copy link
Member

@jochenr @ruromero I've opened a draft PR #37269

@sberyozkin
Copy link
Member

sberyozkin commented Nov 24, 2023

Hi @jochenr @ruromero PR is now ready for review. Please check it and see how it looks.

Apologies it took me a long while to look at it. @ruromero, major thanks for prototyping some code here, I was happy to copy some of it.

@sberyozkin
Copy link
Member

@jochenr Would you like to give #37269 a quick test with your own CN to roles mapping sets or prefer to test against the released version ?

@jochenr
Copy link

jochenr commented Dec 4, 2023

@sberyozkin
Sorry for the delyed response, Since I was/an busy in another projuect, I'll give it a try in the released version.

Thank you very much🍻

@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 5, 2023
@sberyozkin
Copy link
Member

@jochenr Np, thanks for driving this enhancement together with @ruromero 👍

Right now it is targeted at 3.7.0 (3.7.0.CR1 is due in mid of Jan), I've added a backport label to 3.2.x but I'm not sure when this backport will happen

@jochenr
Copy link

jochenr commented Mar 25, 2024

Unfortuntaley today my team recognized that the solution only works for simple use-cases and not more complex ones😥

We have applications that use mTLS for some endpointe and OIDC for other endpoints.

We have something like this

# we have to use REQUEST, bacause with REQUIRED the OIDC path won't work
quarkus.http.ssl.client-auth=REQUEST
quarkus.http.auth.permission.certauthenticated.paths=/secured/mtls/*

# What policy do i have to use if I just want the "quarkus.http.auth.certificate-role-properties=role-mappings.txt" here?
# with just "authenticated" the "certificate-role-properties" are ignored.
quarkus.http.auth.permission.certauthenticated.policy=authenticated

# How can I map this to "quarkus.http.auth.permission.certauthenticated.paths=/secured/mtls/*" only?
quarkus.http.auth.certificate-role-properties=role-mappings.txt


# OTHER endpoints in the same application

quarkus.oidc.enabled=true
quarkus.oidc.tenant-id=......

[...]
quarkus.http.auth.policy.role-policy1.roles-allowed=user,admin

quarkus.http.auth.permission.roles1.paths=/secured/oidc
quarkus.http.auth.permission.roles1.policy=role-policy1
quarkus.http.auth.permission.roles1.auth-mechanism=bearer
[...]


# everyone is allowed to access the pulic path
quarkus.http.auth.permission.public.paths=/public/*
quarkus.http.auth.permission.public.policy=permit

[...]


I think it would be better to have the same path-roles-policy relationship as with the other authentication types.

Sory for not noticing this earlier. I did only tests with our basic/simple applications😥

@michalvavrik
Copy link
Member

michalvavrik commented Mar 25, 2024

@jochenr problem with your example is that I'm not sure I completely understand, however let me try:

How can I map this to "quarkus.http.auth.permission.certauthenticated.paths=/secured/mtls/*" only?

You would need mTLS only applied on /secured/mtls/*, that is to select mTLS with quarkus.http.auth.permission.cerauthenticated.auth-mechanism=X509

I'm sure this will require additional explanation, please feel free to correct me where I misunderstood you.

@michalvavrik
Copy link
Member

michalvavrik commented Mar 25, 2024

What policy do i have to use if I just want the "quarkus.http.auth.certificate-role-properties=role-mappings.txt" here?
with just "authenticated" the "certificate-role-properties" are ignored.

this feature is not related to policies, but you need to authenticate to have roles, so.. require authentication. but if you use RolesAllowed annotation, you don't need any policy there. in fact, I bet using policy permit for /secured/mtls/* should work as long as you select the mechanism, but authenticated policy makes better sense

@jochenr
Copy link

jochenr commented Mar 25, 2024

Hi @michalvavrik,

yes, I'd expect that ist works somehow like this

quarkus.http.ssl.client-auth=REQUEST

quarkus.http.auth.permission.certauthenticated.paths=/secured/mtls/*
quarkus.http.auth.permission.certauthenticated.auth-mechanism=X509
quarkus.http.auth.permission.certauthenticated.policy=role-policy-cert

quarkus.http.auth.policy.role-policy-cert.roles-allowed=user,admin

I prefer using this an not the @RolesAllowed annotation, becaus ethe endpoints are provided as Camel-Rest endpoints (and not JAX-RS)

@michalvavrik
Copy link
Member

cc @sberyozkin

yes, I'd expect that ist works somehow like this

Try this:

quarkus.http.auth.policy.role-policy-cert.roles-allowed=user,admin
quarkus.http.ssl.client-auth=REQUEST

# select mechanism
quarkus.http.auth.permission.certauthenticated.paths=/secured/mtls/*
quarkus.http.auth.permission.certauthenticated.auth-mechanism=X509
quarkus.http.auth.permission.certauthenticated.policy=permit

# or (alternative option) because you already know that request MUST be authenticated anyway (they need roles)
quarkus.http.auth.permission.certauthenticated.policy=authenticated

I prefer using this an not the @RolesAllowed annotation, becaus ethe endpoints are provided as Camel-Rest endpoints (and not JAX-RS)

that part is fine, we believe that's how most people use it. I have prepared annotation-based selection for authentication mechanisms here #36504 where you might do this

@Path
@GET
@RolesAllowed
@MTLS
public String endpoint() {
...

but that might never get in.

@michalvavrik
Copy link
Member

michalvavrik commented Mar 25, 2024

Camel-Rest endpoints (and not JAX-RS)

ah sorry, I completely misread your note about annotations (missed NOT) as I dont' know camel-rest endpoints. Okay, in that case you need exactly what your wrote with quarkus.http.auth.policy.role-policy-cert.roles-allowed=user,admin. Doesn't it work? if not, it's a bug

@jochenr
Copy link

jochenr commented Mar 25, 2024

ah, now I get it...and it works👍😃
Thank you

@michalvavrik
Copy link
Member

Cool, glad it works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants