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

feat: Extraction of thumbprint value through the certificate #389

Closed

JhontSouth wants to merge 4 commits into main from southworks/add/extract-thumbprint-from-certificate

JhontSouth commented Dec 7, 2023 •

edited

Loading

#minor

Description

This PR allows the creation of an object of the CertificateServiceClientCredentialsFactory class using only the values:

App Id
x5c
Certificate key

In addition to being able to get the thumbprint value through the certificate when creating the credentials.

Specific Changes

Added the possibility of two possible parameters (thumbprint of x5c) for the constructor of the CertificateServiceClientCredentialsFactory class.
Added the method getThumbprint to obtain the thumbprint value of the certificate when the x5c value is provided.

Testing

The following image shows the bot running with SNI authentication.

This is the configuration required to use the SNI authentication.


          extract thumbprint from cert value

0cc7414

JhontSouth requested review from ceciliaavila and sw-joelmut

December 7, 2023 13:03

JhontSouth changed the title ~~Extraction of thumbprint value through the certificate~~ feat: Extraction of thumbprint value through the certificate

coveralls commented Dec 7, 2023 •

edited

Loading

Pull Request Test Coverage Report for Build 7131470459

7 of 10 (70.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.009%) to 84.636%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts	7	10	70.0%

Totals
Change from base Build 7049677954:	-0.009%
Covered Lines:	20350
Relevant Lines:	22777

💛 - Coveralls

ceciliaavila requested changes

View reviewed changes

libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts Outdated Show resolved Hide resolved

libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts Outdated Show resolved Hide resolved

ceciliaavila approved these changes

View reviewed changes

libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts Show resolved Hide resolved

sw-joelmut requested changes

View reviewed changes

libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts Outdated

Comment on lines 84 to 87

+                      const certString = Buffer.from(cert).toString();
+                      const begin = certString.lastIndexOf('-----BEGIN CERTIFICATE-----');
+                      const end = certString.lastIndexOf('-----END CERTIFICATE-----') + '-----END CERTIFICATE-----'.length;
+                      const certificate = certString.slice(begin, end);

sw-joelmut Dec 7, 2023

Do we need to pass the -----BEGIN CERTIFICATE----- and -----END CERTIFICATE----- into the openssl function?

If we dont have to, we could just pass the public certificate value by doing like MSAL is doing internally /-----BEGIN CERTIFICATE-----\r*\n(.+?)\r*\n-----END CERTIFICATE-----/gs.
If we need to pass the public cert with the begin and end tags, we could maintain this logic, but put the BEGIN and END tags in variables
Can the certificate just be past as it is to the openssl function?, i see there is a Fingerprint thats being remove after the function execution, if passing the entire certificate as it is, couldnt we just look for that fingerprint and extract its value using for example regex?

libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts Outdated Show resolved Hide resolved

libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts Outdated

+                   * @param tenantId Optional. The oauth token tenant.
+                   * set this parameter to send the public certificate (BEGIN CERTIFICATE) to Azure AD, so that Azure AD can use it to validate the subject name based on a trusted issuer policy.
+                   */
+                  constructor(appId: string, certificateThumbprintOrx5c?: string, certificatePrivateKey?: string, tenantId?: string);

sw-joelmut Dec 7, 2023

The naming of the parameters are confusing here, this naming is only appropriate in the constructor implementation, not the constructor overloading that is visible to the client.

sw-joelmut Dec 7, 2023

I believe that this method is the one that doesnt have the thumbprint, if we can maintain the original constructor behavior in this one by just not including the thumbprint will be ideal.
If for some reason we cant do that, we could move the x5c parameter in the place of the thumbprint which i think you did here.

libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts Outdated

Comment on lines 52 to 70

    
                  constructor(appId: string, certificateThumbprintOrx5c?: string, certificatePrivateKey?: string, tenantId?: string) {

                      super();

                      ok(appId?.trim(), 'CertificateServiceClientCredentialsFactory.constructor(): missing appId.');

                      ok(

                          certificateThumbprint?.trim(),

                          'CertificateServiceClientCredentialsFactory.constructor(): missing certificateThumbprint.'

                          certificateThumbprintOrx5c?.trim(),

                          'CertificateServiceClientCredentialsFactory.constructor(): missing certificateThumbprint or x5c value.'

                      );

                      ok(

                          certificatePrivateKey?.trim(),

                          'CertificateServiceClientCredentialsFactory.constructor(): missing certificatePrivateKey.'

                      );

                      this.appId = appId;

                      this.certificateThumbprint = certificateThumbprint;

                      this.certificateThumbprint = certificateThumbprintOrx5c?.length <= 40 ? certificateThumbprintOrx5c : undefined;

                      this.certificatePrivateKey = certificatePrivateKey;

                      this.tenantId = tenantId;

                      this.x5c = x5c;

                      this.x5c = certificateThumbprintOrx5c?.length > 40 ? certificateThumbprintOrx5c : undefined;

                  }

sw-joelmut Dec 7, 2023

By changing the declaration of the previous constructors, this implementation will also change.
The use of the condition <= 40 is not really good in my opinion to determine if it is a thumbprint or a x5c, this size could change in a future, and could start failing without knowing. We need to find a better approach.
An idea i just think of is to, leave the original constructor as it was and just create the new one, it will be a difference of 1 parameter, if that parameter is undefined use one or the other, the only problem i see with this is that the user pass undefined in that last variable when some external function retrieved the x5c value as undefined an just pass it to this function.
Another option could be detecting if the value passed in the parameter is a x5c, i believe that this we already have it, detecting if it has the BEGIN CERTIFICATE or something like that.
Another option could be combining the two

Author

JhontSouth Dec 7, 2023

What do you mean with "leave the original constructor as it was and just create the new one"?
I have two options for this:

Leave the parameters like the previous one:
(appId: string, certificateThumbprint: string, certificatePrivateKey: string, tenantId?: string, x5c: string,)
And the user will have to send this ("appId", "", "key", "tenant", "x5c")
Use the includes function with BEGIN CERTIFICATE to validate the x5c value

I would rather the second one

sw-joelmut requested changes

View reviewed changes

libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts Outdated

Comment on lines 76 to 79

+                          ok(
+                              certificateThumbprintOrx5c.trim(),
+                              'CertificateServiceClientCredentialsFactory.constructor(): missing x5c.'
+                          );

sw-joelmut Dec 7, 2023

now looking at this, this validation will not be triggered anymore, because will fail if the certificateThumbprintOrx5c is empty, which will not be the case.

And add in the else in the validation the or x5c like you had before because we dont know if it is the thumbprint or the x5c

ceciliaavila approved these changes

View reviewed changes

ceciliaavila left a comment

One more comment to apply from Joel. The rest LGTM

JhontSouth added 2 commits

December 7, 2023 11:43


          fix documentation

3c7ed5e


          keep previous constructor with new structure

JhontSouth force-pushed the southworks/add/extract-thumbprint-from-certificate branch from 4927b70 to 8803011 Compare

December 7, 2023 16:49

sw-joelmut approved these changes

View reviewed changes


          include validation to avoid undefined.trim()

133c9bc

Author

JhontSouth commented Dec 7, 2023

Promoted /4580

JhontSouth closed this

JhontSouth deleted the southworks/add/extract-thumbprint-from-certificate branch

December 7, 2023 17:12

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

Labels

None yet