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

feat: Extraction of thumbprint value through the certificate #389

Closed
wants to merge 4 commits into from
Closed

feat: Extraction of thumbprint value through the certificate #389

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0cc7414
extract thumbprint from cert value
JhontSouth Dec 7, 2023
3c7ed5e
fix documentation
JhontSouth Dec 7, 2023
8803011
keep previous constructor with new structure
JhontSouth Dec 7, 2023
133c9bc
include validation to avoid undefined.trim()
JhontSouth Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion libraries/botframework-connector/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
"cross-fetch": "^3.0.5",
"jsonwebtoken": "^9.0.0",
"rsa-pem-from-mod-exp": "^0.8.4",
"zod": "^3.22.4"
"zod": "^3.22.4",
"openssl-wrapper": "^0.3.4"
},
"devDependencies": {
"@types/jsonwebtoken": "8.3.5",
Expand Down
56 changes: 43 additions & 13 deletions libraries/botframework-connector/src/auth/certificateServiceClientCredentialsFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import type { ServiceClientCredentials } from '@azure/ms-rest-js';
import { ServiceClientCredentialsFactory } from './serviceClientCredentialsFactory';
import { ok } from 'assert';
import { CertificateAppCredentials } from './certificateAppCredentials';
import { promisify } from 'util';
import * as opensslWrapper from 'openssl-wrapper';
const openssl = promisify(opensslWrapper.default);

/**
* A Certificate implementation of the [ServiceClientCredentialsFactory](xref:botframework-connector.ServiceClientCredentialsFactory) abstract class.
Expand All @@ -28,32 +31,42 @@ export class CertificateServiceClientCredentialsFactory extends ServiceClientCre
* @param certificateThumbprint A hex encoded thumbprint of the certificate.
* @param certificatePrivateKey A PEM encoded certificate private key.
* @param tenantId Optional. The oauth token tenant.
* @param x5c Optional. Enables application developers to achieve easy certificates roll-over in Azure AD:
* 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,
certificateThumbprint: string,
certificatePrivateKey: string,
tenantId?: string,
x5c?: string
) {
constructor(appId: string, certificateThumbprintOrx5c?: string, certificatePrivateKey?: string, tenantId?: string);

ceciliaavila marked this conversation as resolved.
Show resolved Hide resolved
/**
* Initializes a new instance of the CertificateServiceClientCredentialsFactory class.
*
* @param appId Microsoft application Id related to the certificate.
* @param x5c Value that enables application developers to achieve easy certificates roll-over in Azure AD
* @param certificatePrivateKey A PEM encoded certificate private key.
* @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);
Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.


/**
* @internal
*/
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;
}
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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


/**
Expand All @@ -63,6 +76,23 @@ export class CertificateServiceClientCredentialsFactory extends ServiceClientCre
return appId === this.appId;
}

/**
* @param cert Value with the certificate content.
* @returns the thumbprint value calculated from the cert content.
ceciliaavila marked this conversation as resolved.
Show resolved Hide resolved
*/
async getThumbprint(cert) {
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);
Copy link

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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
  3. 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?


const fingerprintResponse = await openssl('x509', Buffer.from(certificate), { fingerprint: true, noout: true });
return Buffer.from(fingerprintResponse)
.toString()
.replace(/^.*Fingerprint=/, '')
.replace(/:/g, '')
.trim();
}
/**
* @inheritdoc
*/
Expand All @@ -82,7 +112,7 @@ export class CertificateServiceClientCredentialsFactory extends ServiceClientCre

return new CertificateAppCredentials(
this.appId,
this.certificateThumbprint,
this.certificateThumbprint ?? (await this.getThumbprint(this.x5c)),
this.certificatePrivateKey,
this.tenantId,
audience,
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10254,6 +10254,11 @@ [email protected], open@^8.0.0:
is-docker "^2.1.1"
is-wsl "^2.2.0"

openssl-wrapper@^0.3.4:
version "0.3.4"
resolved "https://registry.yarnpkg.com/openssl-wrapper/-/openssl-wrapper-0.3.4.tgz#c01ec98e4dcd2b5dfe0b693f31827200e3b81b07"
integrity sha512-iITsrx6Ho8V3/2OVtmZzzX8wQaKAaFXEJQdzoPUZDtyf5jWFlqo+h+OhGT4TATQ47f9ACKHua8nw7Qoy85aeKQ==

optionator@^0.8.1:
version "0.8.3"
resolved "https://registry.yarnpkg.com/optionator/-/optionator-0.8.3.tgz#84fa1d036fe9d3c7e21d99884b601167ec8fb495"
Expand Down
Loading