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.

Sign up for GitHub

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

Jump to bottom

Add IdentityKeyStoreResolver and Util Classes #5794

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Add IdentityKeyStoreResolver and Util Classes #5794

wants to merge 29 commits into from

Conversation

Binara-Sachin
Copy link

@Binara-Sachin Binara-Sachin commented Jul 15, 2024 •
edited

Purpose

  • This PR adds the IdentityKeyStoreResolver class and related util classes.
  • This will allow custom key stores to be defined for each authentication flow.
  • The IdentityKeyStoreResolver class will resolve correct KeyStores, Certificates, and Keys that are to be used in authentication protocols.

deployment.toml configuration

[[identity.keystore.mapping]]
inbound_protocol = "oauth2.0"
keystore_file_name = "custom.jks"
use_in_all_tenants = true

Important

@CLAassistant
Copy link

CLAassistant commented Jul 15, 2024 •
edited

CLA assistant check
All committers have signed the CLA.

@Binara-Sachin Binara-Sachin mentioned this pull request Jul 22, 2024
Open
@Binara-Sachin Binara-Sachin mentioned this pull request Jul 25, 2024
Open
@Binara-Sachin Binara-Sachin marked this pull request as ready for review July 30, 2024 04:39
@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/10382196466

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/10382196466
Status: failure

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/10383018980

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/10383018980
Status: failure

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/10383648364

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/10383648364
Status: failure

pamodaaw
pamodaaw reviewed Aug 14, 2024
View reviewed changes
features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2
Comment on lines 107 to +112
<TrustManagerType>{{key_mgt.trust_manager_type}}</TrustManagerType>
{% if identity.keystore.mapping is defined %}

<CustomKeyStoreMappings>
{% for mapping in identity.keystore.mapping %}
<KeyStoreMapping>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<TrustManagerType>{{key_mgt.trust_manager_type}}</TrustManagerType>
{% if identity.keystore.mapping is defined %}
<CustomKeyStoreMappings>
{% for mapping in identity.keystore.mapping %}
<KeyStoreMapping>
<TrustManagerType>{{key_mgt.trust_manager_type}}</TrustManagerType>
{% if identity.keystore.mapping is defined %}
<CustomKeyStoreMappings>
{% for mapping in identity.keystore.mapping %}
<KeyStoreMapping>

Copy link
Author

Choose a reason for hiding this comment

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

This new line is there to separate the 2 types of configurations

Ex:

<!-- Security configurations -->
    <Security>
        <!-- The directory under which all other KeyStore files will be stored -->
        <KeyStoresDir>${carbon.home}/conf/keystores</KeyStoresDir>
        <KeyManagerType>SunX509</KeyManagerType>
        <TrustManagerType>SunX509</TrustManagerType>

        <CustomKeyStoreMappings>
            <KeyStoreMapping>
                <Protocol>oauth</Protocol>

Should I remove this?

....core/src/test/java/org/wso2/carbon/identity/core/util/IdentityKeyStoreResolverUtilTest.java Outdated Show resolved Hide resolved
....core/src/test/java/org/wso2/carbon/identity/core/util/IdentityKeyStoreResolverUtilTest.java Show resolved Hide resolved
....core/src/test/java/org/wso2/carbon/identity/core/util/IdentityKeyStoreResolverUtilTest.java Outdated Show resolved Hide resolved
....identity.core/src/test/java/org/wso2/carbon/identity/core/IdentityKeyStoreResolverTest.java Show resolved Hide resolved
....identity.core/src/test/java/org/wso2/carbon/identity/core/IdentityKeyStoreResolverTest.java Outdated Show resolved Hide resolved
....identity.core/src/test/java/org/wso2/carbon/identity/core/IdentityKeyStoreResolverTest.java Outdated Show resolved Hide resolved
....identity.core/src/test/java/org/wso2/carbon/identity/core/IdentityKeyStoreResolverTest.java Outdated Show resolved Hide resolved
...rbon.identity.core/src/main/java/org/wso2/carbon/identity/core/IdentityKeyStoreResolver.java Outdated Show resolved Hide resolved
...core/src/main/java/org/wso2/carbon/identity/core/util/IdentityKeyStoreResolverConstants.java Outdated Show resolved Hide resolved
pamodaaw
pamodaaw reviewed Aug 14, 2024
View reviewed changes
...tity.core/src/main/java/org/wso2/carbon/identity/core/util/IdentityKeyStoreResolverUtil.java Show resolved Hide resolved
...tity.core/src/main/java/org/wso2/carbon/identity/core/util/IdentityKeyStoreResolverUtil.java Show resolved Hide resolved
....identity.core/src/test/java/org/wso2/carbon/identity/core/IdentityKeyStoreResolverTest.java Outdated Show resolved Hide resolved
....identity.core/src/test/java/org/wso2/carbon/identity/core/IdentityKeyStoreResolverTest.java Show resolved Hide resolved
....identity.core/src/test/java/org/wso2/carbon/identity/core/IdentityKeyStoreResolverTest.java Show resolved Hide resolved
....identity.core/src/test/java/org/wso2/carbon/identity/core/IdentityKeyStoreResolverTest.java Outdated
assertEquals(tenantCertificate, identityKeyStoreResolver.getCertificate(TENANT_DOMAIN, InboundProtocol.WS_TRUST));
}

private void setFinalStatic(Field field, Object newValue) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain the use of this method

Copy link
Author

Choose a reason for hiding this comment

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

Used this method to set values to private static variables in IdentityKeyStoreResolver class

Copy link
Author

Choose a reason for hiding this comment

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

Improved by moving the logic to setPrivateStaticField method

pamodaaw
pamodaaw reviewed Aug 20, 2024
View reviewed changes
components/identity-core/org.wso2.carbon.identity.core/src/test/resources/testng.xml
@@ -28,6 +28,7 @@
<class name="org.wso2.carbon.identity.core.internal.DefaultServiceURLBuilderTest"/>
<class name="org.wso2.carbon.identity.core.cache.BaseCacheTest"/>
<class name="org.wso2.carbon.identity.core.ThreadLocalAwareThreadPoolExecutorTest"/>
<class name="org.wso2.carbon.identity.core.IdentityKeyStoreResolverTest"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the util test class?

pamodaaw
pamodaaw reviewed Aug 20, 2024
View reviewed changes
....identity.core/src/test/java/org/wso2/carbon/identity/core/IdentityKeyStoreResolverTest.java
keyStoreMappings.put(InboundProtocol.OAUTH, oauthKeyStoreMapping);
keyStoreMappings.put(InboundProtocol.WS_TRUST, wsTrustKeyStoreMapping);

setPrivateStaticField(IdentityKeyStoreResolver.class, "keyStoreMappings", keyStoreMappings);
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we have a setter method to set values for this attribute?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pamodaaw pamodaaw pamodaaw left review comments

@DInuwan97 DInuwan97 DInuwan97 left review comments

@ImalshaG ImalshaG ImalshaG left review comments

@lakshithakasun lakshithakasun lakshithakasun left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Binara-Sachin @CLAassistant @jenkins-is-staging @pamodaaw @DInuwan97 @ImalshaG @lakshithakasun