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

[Identity] What if we accepted the PEM certificate string contents? #18017

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
6 changes: 6 additions & 0 deletions sdk/identity/identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,14 @@

### Features Added

- `ClientCertificateCredential` now accepts the string contents of the PEM certificate besides the path to the PEM certificate through the new type `ClientCertificateCredentialPEMConfiguration`.

### Breaking Changes

#### Breaking changes from 2.0.0-beta.6 and 1.5.2

- `ClientCertificateCredential` now won't accept a PEM certificate path as its third parameter. It will accept an object with the type `ClientCertificateCredentialPEMConfiguration` containing either a property named `certificate` with the string contents of the PEM certificate, or a property named `certificatePath`, with the path to the PEM certificate.

#### Breaking Changes from 2.0.0-beta.4

- Removed the `allowMultiTenantAuthentication` option from all of the credentials. Multi-tenant authentication is now enabled by default. On Node.js, it can be disabled with the `AZURE_IDENTITY_DISABLE_MULTITENANTAUTH` environment variable.
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion sdk/identity/identity/review/identity.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ export class ChainedTokenCredential implements TokenCredential {

// @public
export class ClientCertificateCredential implements TokenCredential {
constructor(tenantId: string, clientId: string, certificatePath: string, options?: ClientCertificateCredentialOptions);
// Warning: (ae-forgotten-export) The symbol "ClientCertificateCredentialPEMConfiguration" needs to be exported by the entry point index.d.ts
sadasant marked this conversation as resolved.
Show resolved Hide resolved
constructor(tenantId: string, clientId: string, configuration: ClientCertificateCredentialPEMConfiguration, options?: ClientCertificateCredentialOptions);
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken>;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,33 @@ import { trace } from "../util/tracing";
import { MsalFlow } from "../msal/flows";
import { ClientCertificateCredentialOptions } from "./clientCertificateCredentialOptions";

const logger = credentialLogger("ClientCertificateCredential");
const credentialName = "ClientCertificateCredential";
const logger = credentialLogger(credentialName);

/**
* Required configuration options for the {@link ClientCertificateCredential}, with either the string contents of a PEM certificate, or the path to a PEM certificate.
sadasant marked this conversation as resolved.
Show resolved Hide resolved
*/
export type ClientCertificateCredentialPEMConfiguration =
| {
/**
* The PEM-encoded public/private key certificate on the filesystem.
*/
certificate: string;
/**
* The PEM-encoded public/private key certificate on the filesystem should not be provided if `certificate` is provided.
*/
certificatePath?: never;
Copy link
Member

Choose a reason for hiding this comment

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

this is clever to avoid folks providing both on accident, at least in TS

Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, I think certificatePath?: undefined; is slightly better. It makes it clear that if you access this value you will get undefined, which is assignable only to other undefined things, whereas never is assignable to any constraint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing that I find weird is that if I wanted to use undefined here I also need to put the question mark, so certificatePath?: undefined, which seems redundant, but it’s necessary for the exclusive-or type to work. Perhaps I’m missing something 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I think what's weird is because we want strictly { a: string } | { b: string }, but because {a: "foo", b: "bar" } satisfies both of them using TS rules, we have to tell it { a: string, b?: undefined } | { a?: undefined, b: string } because we want to support {a: "foo"} and {b: "bar"} (the other prop is not present at all, hence the question mark) but we also want the compiler to complain if the key is present and set to anything except undefined (like a string, which would create the confusion.)

Copy link
Contributor Author

@sadasant sadasant Oct 5, 2021

Choose a reason for hiding this comment

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

@xirzec I think you have captured what’s weird! — I didn’t quite get the suggestion though 🤔

}
| {
/**
* The PEM-encoded public/private key certificate on the filesystem should not be provided if `certificatePath` is provided.
*/
certificate?: never;
/**
* The path to the PEM-encoded public/private key certificate on the filesystem.
*/
certificatePath: string;
};

/**
* Enables authentication to Azure Active Directory using a PEM-encoded
Expand All @@ -28,23 +54,26 @@ export class ClientCertificateCredential implements TokenCredential {
*
* @param tenantId - The Azure Active Directory tenant (directory) ID.
* @param clientId - The client (application) ID of an App Registration in the tenant.
* @param certificatePath - The path to a PEM-encoded public/private key certificate on the filesystem.
* @param configuration - Other parameters required, including the PEM-encoded certificate as a string, or as a path on the filesystem.
* @param options - Options for configuring the client which makes the authentication request.
*/
constructor(
tenantId: string,
clientId: string,
certificatePath: string,
configuration: ClientCertificateCredentialPEMConfiguration,
sadasant marked this conversation as resolved.
Show resolved Hide resolved
options: ClientCertificateCredentialOptions = {}
) {
if (!tenantId || !clientId || !certificatePath) {
if (!tenantId || !clientId) {
throw new Error(`${credentialName}: tenantId and clientId are required parameters.`);
}
if (!configuration || !(configuration.certificate || configuration.certificatePath)) {
throw new Error(
"ClientCertificateCredential: tenantId, clientId, and certificatePath are required parameters."
`${credentialName}: Provide either a PEM certificate in string form, or the path to that certificate in the filesystem.`
);
}
this.msalFlow = new MsalClientCertificate({
...options,
certificatePath,
configuration,
logger,
clientId,
tenantId,
Expand All @@ -62,7 +91,7 @@ export class ClientCertificateCredential implements TokenCredential {
* TokenCredential implementation might make.
*/
async getToken(scopes: string | string[], options: GetTokenOptions = {}): Promise<AccessToken> {
return trace(`${this.constructor.name}.getToken`, options, async (newOptions) => {
return trace(`${credentialName}.getToken`, options, async (newOptions) => {
const arrayScopes = Array.isArray(scopes) ? scopes : [scopes];
return this.msalFlow.getToken(arrayScopes, newOptions);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export class EnvironmentCredential implements TokenCredential {
this._credential = new ClientCertificateCredential(
tenantId,
clientId,
certificatePath,
{ certificatePath },
options
);
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { AccessToken } from "@azure/core-auth";
import { MsalNodeOptions, MsalNode } from "./nodeCommon";
import { formatError } from "../../util/logging";
import { CredentialFlowGetTokenOptions } from "../credentials";
import { ClientCertificateCredentialPEMConfiguration } from "../../credentials/clientCertificateCredential";

const readFileAsync = promisify(readFile);

Expand All @@ -20,7 +21,7 @@ export interface MSALClientCertificateOptions extends MsalNodeOptions {
/**
* Location of the PEM certificate.
*/
certificatePath: string;
configuration: ClientCertificateCredentialPEMConfiguration;
/**
* Option to include x5c header for SubjectName and Issuer name authorization.
* Set this option to send base64 encoded public certificate in the client assertion header as an x5c claim
Expand Down Expand Up @@ -50,18 +51,19 @@ interface CertificateParts {
/**
* Tries to asynchronously load a certificate from the given path.
*
* @param certificatePath - Path to the certificate.
* @param configuration - Either the PEM value or the path to the certificate.
* @param sendCertificateChain - Option to include x5c header for SubjectName and Issuer name authorization.
* @returns - The certificate parts, or `undefined` if the certificate could not be loaded.
* @internal
*/
export async function parseCertificate(
certificatePath: string,
configuration: ClientCertificateCredentialPEMConfiguration,
sendCertificateChain?: boolean
): Promise<CertificateParts> {
const certificateParts: Partial<CertificateParts> = {};

certificateParts.certificateContents = await readFileAsync(certificatePath, "utf8");
certificateParts.certificateContents =
configuration.certificate || (await readFileAsync(configuration.certificatePath!, "utf8"));
if (sendCertificateChain) {
certificateParts.x5c = certificateParts.certificateContents;
}
Expand Down Expand Up @@ -95,20 +97,20 @@ export async function parseCertificate(
* @internal
*/
export class MsalClientCertificate extends MsalNode {
private certificatePath: string;
private configuration: ClientCertificateCredentialPEMConfiguration;
private sendCertificateChain?: boolean;

constructor(options: MSALClientCertificateOptions) {
super(options);
this.requiresConfidential = true;
this.certificatePath = options.certificatePath;
this.configuration = options.configuration;
this.sendCertificateChain = options.sendCertificateChain;
}

// Changing the MSAL configuration asynchronously
async init(options?: CredentialFlowGetTokenOptions): Promise<void> {
try {
const parts = await parseCertificate(this.certificatePath, this.sendCertificateChain);
const parts = await parseCertificate(this.configuration, this.sendCertificateChain);
this.msalConfig.auth.clientCertificate = {
thumbprint: parts.thumbprint,
privateKey: parts.certificateContents,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ export class MsalOnBehalfOf extends MsalNode {
async init(options?: CredentialFlowGetTokenOptions): Promise<void> {
if (this.certificatePath) {
try {
const parts = await parseCertificate(this.certificatePath, this.sendCertificateChain);
const parts = await parseCertificate(
{ certificatePath: this.certificatePath },
this.sendCertificateChain
);
this.msalConfig.auth.clientCertificate = {
thumbprint: parts.thumbprint,
privateKey: parts.certificateContents,
Expand Down
Loading