[#2825] Extend credentials management service to support auth-id template #2985

kaniyan · 2021-11-30T13:26:06Z

Add issuer-DN to credentials management specification.
Extend credentials management service implementations to generate
the auth-id by applying the auth-id-template to the given subject DN.

...rg/eclipse/hono/deviceregistry/service/credentials/AbstractCredentialsManagementService.java

...src/main/java/org/eclipse/hono/service/management/credentials/X509CertificateCredential.java

…auth-id template * Add issuer-DN to credentials management specification. * Extend credentials management service implementations to populate the new field generated-auth-id by applying the auth-id-template to the given subject DN. * During find credentials by type and auth-id operation, the generated-auth-id is looked for any matching value. If there is no value, then the auth-id field is used. Signed-off-by: Kartheeswaran Kalidass <[email protected]>

kaniyan · 2021-12-03T10:45:10Z

@sophokles73 As we discussed, I added a new field generated-auth-id now to support the auth id template feature in JDBC and MongoDB registries. Also the get credentials operation has been extended to support this feature. I have pushed a consolidated commit for it.

sophokles73 · 2021-12-06T07:08:56Z

...ain/java/org/eclipse/hono/deviceregistry/service/credentials/AbstractCredentialsService.java

+                    .map(c -> c.getString(RegistryManagementConstants.FIELD_TYPE))
+                    .filter(RegistryManagementConstants.SECRETS_TYPE_X509_CERT::equals)
+                    .map(ok -> credential.getString(RegistryManagementConstants.FIELD_GENERATED_AUTH_ID))
+                    .ifPresent(id -> credential.put(RegistryManagementConstants.FIELD_AUTH_ID, id));


Since this is the Credentials service, I would assume that all these constants should be defined in CredentialsConstants, right?

...src/main/java/org/eclipse/hono/service/management/credentials/X509CertificateCredential.java

Signed-off-by: Kartheeswaran Kalidass <[email protected]>

sophokles73 · 2021-12-07T09:37:10Z

...src/main/java/org/eclipse/hono/service/management/credentials/X509CertificateCredential.java

+     * @param generatedAuthId the authentication identifier generated by applying the <em>auth-id-template</em>
+     *                        from the tenant's trust anchor to the client certificate's subject DN.
+     * @param secrets The credential's secret(s).
+     * @throws NullPointerException if any of the parameters are {@code null}.


the code seems to suggest that issuer DN and generatedAuthId can actually be null

sophokles73 · 2021-12-07T09:38:47Z

...src/main/java/org/eclipse/hono/service/management/credentials/X509CertificateCredential.java

-     * @throws NullPointerException if any of the parameters are {@code null}.
-     * @throws IllegalArgumentException if the given string is not a valid X.500 distinguished name or if
-     *                                  secrets is empty.
+     * @throws NullPointerException if subject DN and secrets are {@code null}.


subject DN or secrets are null?

sophokles73 · 2021-12-07T09:42:38Z

...src/main/java/org/eclipse/hono/service/management/credentials/X509CertificateCredential.java

     * @param secrets The credential's secret(s).
-     * @throws NullPointerException if certificate bytes and any of distinguished name and secrets are {@code null}.
+     * @throws NullPointerException if certificate bytes, subject DN and secrets are {@code null}.


IMHO the original wording was correct

The distinguished name in the old wording is ambiguous as both issuer-dn and subject-dn are distinguished names.
How about if certificate bytes and any of subject DN and secrets are {@code null}?

it is not about distinguished name vs subject DN. it is about the expression built from the three parameters.

if certificate bytes and any of distinguished name and secrets are {@code null}

is true if either certificate bytes AND distinguished name are null, OR if certificate bytes AND secrets are null

but

if certificate bytes, subject DN and secrets are {@code null}.

is only true if all three parameters are null, or am I mistaken?

if certificate bytes, subject DN and secrets are {@code null}.

We agree on the same. The above one is not correct.

In my previous comment, I proposed the below one. I just replaced distinguished name with subject DN in the original description as we now have two distinguished names in the list of paramteres(final String subjectDN, final String issuerDN).

if certificate bytes and any of subject DN and secrets are {@code null}

Another suggestions is as below:

if certificate bytes and either subject DN or secrets are {@code null}

Or am I missing something?

...godb/src/main/java/org/eclipse/hono/deviceregistry/mongodb/utils/MongoDbDocumentBuilder.java

sophokles73 · 2021-12-07T09:49:03Z

...godb/src/main/java/org/eclipse/hono/deviceregistry/mongodb/utils/MongoDbDocumentBuilder.java

+        final var credentialsArraySpec = document.getJsonObject(CredentialsDto.FIELD_CREDENTIALS, new JsonObject());
+        final var elementMatchSpec = credentialsArraySpec.getJsonObject(MONGODB_OPERATOR_ELEM_MATCH, new JsonObject());
+
+        elementMatchSpec.put(MONGODB_OPERATOR_OR, new JsonArray(List.of(


no need to create a List instance here

I think a List is needed there as there are two JsonObjects to be added to the JsonArray. If we like to avoid using List, then it can be rewritten as below. One way of populating JsonArray is to pass a list to the constructor and another is to invoke add methods.

elementMatchSpec.put(MONGODB_OPERATOR_OR, new JsonArray().add(new JsonObject()).add(new JsonObject())

sophokles73 · 2021-12-07T13:43:15Z

core/src/main/java/org/eclipse/hono/util/CredentialsConstants.java

+     * The name of the field that contains the generated authentication identifier
+     * by applying the template to the subject DN.
+     */
+    public static final String FIELD_GENERATED_AUTH_ID           = "generated-auth-id";


do we still need this constant now that we do not expose the generated ID field anymore?

Yes it is still being used in CredentialsServiceImpl.

sophokles73 · 2021-12-07T14:49:25Z

...src/main/java/org/eclipse/hono/service/management/credentials/X509CertificateCredential.java

+    public final X509CertificateCredential applyAuthIdTemplate(final Tenant tenant) {
+        Objects.requireNonNull(tenant, "tenant information must not be null");
+
+        final String generatedAuthId = Optional.ofNullable(issuerDN)
+                .flatMap(tenant::getAuthIdTemplate)
+                .map(IdentityTemplate::new)
+                .map(t -> t.apply(getAuthId()))
+                .orElse(null);
+        final X509CertificateCredential credential = new X509CertificateCredential(getAuthId(),
+                generatedAuthId, secrets) {
+
+            @Override
+            @JsonGetter(value = RegistryManagementConstants.FIELD_GENERATED_AUTH_ID)
+            protected String getGeneratedAuthId() {
+                return super.getGeneratedAuthId();
+            }
+        };
+
+        copyCommonAttributesTo(credential);
+        return credential;
+    }


I guess that this works but, pardon my French, it looks like a hack. Wouldn't it be easier to understand and also much cleaner from a design perspective to introduce two subclasses instead, one for handling credentials with a generated ID and the other for a non-generated ID?

I noticed that anonymous class approach is employed in several places in Hono. I chose that approach than my initial one. Good that I still have a backup of X509CertificateCredentialWithGeneratedAuthId ;-)

Signed-off-by: Kartheeswaran Kalidass <[email protected]>

kaniyan requested a review from sophokles73 November 30, 2021 13:26

kaniyan requested a review from ctron as a code owner November 30, 2021 13:26

kaniyan added this to the 1.11.0 milestone Nov 30, 2021

kaniyan added the Device Registry label Nov 30, 2021

sophokles73 reviewed Nov 30, 2021

View reviewed changes

...rg/eclipse/hono/deviceregistry/service/credentials/AbstractCredentialsManagementService.java Outdated Show resolved Hide resolved

...src/main/java/org/eclipse/hono/service/management/credentials/X509CertificateCredential.java Outdated Show resolved Hide resolved

kaniyan force-pushed the PR/#2825_part_2_authId_template branch from 2c9c5e8 to c2c01be Compare November 30, 2021 15:24

kaniyan force-pushed the PR/#2825_part_2_authId_template branch from c2c01be to a9af944 Compare December 3, 2021 10:26

sophokles73 reviewed Dec 6, 2021

View reviewed changes

sophokles73 force-pushed the master branch from eba2374 to c8005d6 Compare December 6, 2021 16:00

Incorporate review comments and minor improvements

142b98a

Signed-off-by: Kartheeswaran Kalidass <[email protected]>

kaniyan force-pushed the PR/#2825_part_2_authId_template branch from 973bd2b to 142b98a Compare December 6, 2021 18:49

sophokles73 reviewed Dec 7, 2021

View reviewed changes

kaniyan force-pushed the PR/#2825_part_2_authId_template branch from 6e5e0b7 to 88c61ce Compare December 8, 2021 08:23

Add tests and incorporate review comments

bd060cd

Signed-off-by: Kartheeswaran Kalidass <[email protected]>

kaniyan force-pushed the PR/#2825_part_2_authId_template branch from 88c61ce to bd060cd Compare December 8, 2021 08:30

kaniyan mentioned this pull request Dec 8, 2021

[#2825] Make use of auth-id template to get credentials #2968

Closed

sophokles73 removed this from the 1.11.0 milestone Jan 10, 2022

sophokles73 added this to the 1.12.0 milestone Jan 10, 2022

sophokles73 removed this from the 1.12.0 milestone Feb 10, 2022

kaniyan requested a review from dejanb as a code owner February 23, 2022 13:17

n-deliyski mentioned this pull request Jan 20, 2023

Possibility to provide auth-ID pattern as tenant configuration #2825

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2825] Extend credentials management service to support auth-id template #2985

[#2825] Extend credentials management service to support auth-id template #2985

kaniyan commented Nov 30, 2021

kaniyan commented Dec 3, 2021

sophokles73 Dec 6, 2021

sophokles73 Dec 7, 2021

sophokles73 Dec 7, 2021

sophokles73 Dec 7, 2021

kaniyan Dec 7, 2021

sophokles73 Dec 7, 2021

kaniyan Dec 7, 2021

sophokles73 Dec 7, 2021

kaniyan Dec 7, 2021

sophokles73 Dec 7, 2021

kaniyan Dec 7, 2021

sophokles73 Dec 7, 2021

kaniyan Dec 7, 2021

[#2825] Extend credentials management service to support auth-id template #2985

Are you sure you want to change the base?

[#2825] Extend credentials management service to support auth-id template #2985

Conversation

kaniyan commented Nov 30, 2021

kaniyan commented Dec 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment