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 all commits
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
1 change: 1 addition & 0 deletions sdk/identity/identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ Azure Service Fabric support hasn't been added on the initial version 2 of Ident

#### Other features

- `ClientCertificateCredential` now optionally accepts a configuration object as its third constructor parameter, instead of the PEM certificate path. This new object, called `ClientCertificateCredentialPEMConfiguration`, can contain either the PEM certificate path with the `certificatePath` property, or the contents of the PEM certificate with the `certificate` property..
- The Node.js version of `InteractiveBrowserCredential` has [Proof Key for Code Exchange (PKCE)](https://datatracker.ietf.org/doc/html/rfc7636) enabled by default.
- `InteractiveBrowserCredential` has a new `loginHint` constructor option, which allows a username to be pre-selected for interactive logins.
- In `AzureCliCredential`, we allow specifying a `tenantId` in the parameters through the `AzureCliCredentialOptions`.
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.

10 changes: 10 additions & 0 deletions sdk/identity/identity/review/identity.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export class ChainedTokenCredential implements TokenCredential {
// @public
export class ClientCertificateCredential implements TokenCredential {
constructor(tenantId: string, clientId: string, certificatePath: string, options?: ClientCertificateCredentialOptions);
constructor(tenantId: string, clientId: string, configuration: ClientCertificateCredentialPEMConfiguration, options?: ClientCertificateCredentialOptions);
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken>;
}

Expand All @@ -117,6 +118,15 @@ export interface ClientCertificateCredentialOptions extends TokenCredentialOptio
sendCertificateChain?: boolean;
}

// @public
export type ClientCertificateCredentialPEMConfiguration = {
certificate: string;
certificatePath?: never;
} | {
certificate?: never;
certificatePath: string;
};

// @public
export class ClientSecretCredential implements TokenCredential {
constructor(tenantId: string, clientId: string, clientSecret: string, options?: ClientSecretCredentialOptions);
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 @@ -35,16 +61,53 @@ export class ClientCertificateCredential implements TokenCredential {
tenantId: string,
clientId: string,
certificatePath: string,
options?: ClientCertificateCredentialOptions
);
/**
* Creates an instance of the ClientCertificateCredential with the details
* needed to authenticate against Azure Active Directory with a certificate.
*
* @param tenantId - The Azure Active Directory tenant (directory) ID.
* @param clientId - The client (application) ID of an App Registration in the tenant.
* @param configuration - Other parameters required, including the PEM-encoded certificate as a string, or as a path on the filesystem.
* If the type is ignored, we will throw if both the value of the PEM certificate and the path to a PEM certificate are provided at the same time.
* @param options - Options for configuring the client which makes the authentication request.
*/
constructor(
tenantId: string,
clientId: string,
configuration: ClientCertificateCredentialPEMConfiguration,
sadasant marked this conversation as resolved.
Show resolved Hide resolved
options?: ClientCertificateCredentialOptions
);
constructor(
tenantId: string,
clientId: string,
certificatePathOrConfiguration: string | ClientCertificateCredentialPEMConfiguration,
options: ClientCertificateCredentialOptions = {}
) {
if (!tenantId || !clientId || !certificatePath) {
if (!tenantId || !clientId) {
throw new Error(`${credentialName}: tenantId and clientId are required parameters.`);
}
const configuration: ClientCertificateCredentialPEMConfiguration = {
...(typeof certificatePathOrConfiguration === "string"
? {
certificatePath: certificatePathOrConfiguration
}
: certificatePathOrConfiguration)
};
if (!configuration || !(configuration.certificate || configuration.certificatePath)) {
throw new Error(
`${credentialName}: Provide either a PEM certificate in string form, or the path to that certificate in the filesystem.`
);
}
if (configuration.certificate && configuration.certificatePath) {
throw new Error(
"ClientCertificateCredential: tenantId, clientId, and certificatePath are required parameters."
`${credentialName}: To avoid unexpected behaviors, providing both the contents of a PEM certificate and the path to a PEM certificate is forbidden.`
);
}
this.msalFlow = new MsalClientCertificate({
...options,
certificatePath,
configuration,
logger,
clientId,
tenantId,
Expand All @@ -62,7 +125,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 @@ -98,7 +98,7 @@ export class EnvironmentCredential implements TokenCredential {
this._credential = new ClientCertificateCredential(
tenantId,
clientId,
certificatePath,
{ certificatePath },
options
);
return;
Expand Down
5 changes: 4 additions & 1 deletion sdk/identity/identity/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ export {
} from "./credentials/environmentCredential";
export { ClientSecretCredential } from "./credentials/clientSecretCredential";
export { ClientSecretCredentialOptions } from "./credentials/clientSecretCredentialOptions";
export { ClientCertificateCredential } from "./credentials/clientCertificateCredential";
export {
ClientCertificateCredential,
ClientCertificateCredentialPEMConfiguration
} from "./credentials/clientCertificateCredential";
export { ClientCertificateCredentialOptions } from "./credentials/clientCertificateCredentialOptions";
export { CredentialPersistenceOptions } from "./credentials/credentialPersistenceOptions";
export { AzureCliCredential } from "./credentials/azureCliCredential";
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