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

Use creds as supplementary of individual parameters #328

Closed
wants to merge 9 commits into from

Conversation

MoChilia
Copy link
Member

@MoChilia MoChilia commented May 29, 2023

Description

This pr is going to read creds from Json, the items in creds will not overwrite the individual parameters client-id, tenant-id and subscription-id, but as supplementary.

  • In the previous code, creds is not compatible with individual parameters. We use creds for login using service principal with secret and individual parameters for OIDC login:

    login/src/main.ts

    Lines 78 to 93 in 990b22f

    if (servicePrincipalId || tenantId || subscriptionId) {
    //If few of the individual credentials (clent_id, tenat_id, subscription_id) are missing in action inputs.
    if (!(servicePrincipalId && tenantId && (subscriptionId || allowNoSubscriptionsLogin)))
    throw new Error("Few credentials are missing. ClientId, tenantId are mandatory. SubscriptionId is also mandatory if allow-no-subscriptions is not set.");
    }
    else {
    if (creds) {
    core.debug('using creds JSON...');
    enableOIDC = false;
    servicePrincipalId = secrets.getSecret("$.clientId", true);
    servicePrincipalKey = secrets.getSecret("$.clientSecret", true);
    tenantId = secrets.getSecret("$.tenantId", true);
    subscriptionId = secrets.getSecret("$.subscriptionId", true);
    resourceManagerEndpointUrl = secrets.getSecret("$.resourceManagerEndpointUrl", false);
    }

    In the new version, we aim to fetch the user's input credentials as much as possible:
    if (creds) {
    core.debug('Reading creds in JSON...');
    this.servicePrincipalId = this.servicePrincipalId ? this.servicePrincipalId : secrets.getSecret("$.clientId", false);
    this.servicePrincipalKey = secrets.getSecret("$.clientSecret", false);
    this.tenantId = this.tenantId ? this.tenantId : secrets.getSecret("$.tenantId", false);
    this.subscriptionId = this.subscriptionId ? this.subscriptionId : secrets.getSecret("$.subscriptionId", false);
    this.resourceManagerEndpointUrl = secrets.getSecret("$.resourceManagerEndpointUrl", false);
    }

  • We remove the boolean enableOIDC and use servicePrincipalKey to decide whether OIDC is enabled.

  • We also remove the adjudication for undefined federatedToken in L122, since the error will be caught in L118. (Related to Support OIDC for sovereign clouds #321)

    login/src/main.ts

    Lines 115 to 129 in 990b22f

    try{
    federatedToken = await core.getIDToken(audience);
    }
    catch (error) {
    core.error(`Please make sure to give write permissions to id-token in the workflow.`);
    throw error;
    }
    if (!!federatedToken) {
    let [issuer, subjectClaim] = await jwtParser(federatedToken);
    console.log("Federated token details: \n issuer - " + issuer + " \n subject claim - " + subjectClaim);
    }
    else{
    throw new Error("Failed to fetch federated token.");
    }
    }

@MoChilia MoChilia requested review from jiasli, evelyn-ys and YanaXu May 29, 2023 05:41
@MoChilia MoChilia temporarily deployed to Automation test May 29, 2023 05:41 — with GitHub Actions Inactive
@MoChilia MoChilia temporarily deployed to Automation test May 29, 2023 08:59 — with GitHub Actions Inactive
@MoChilia MoChilia marked this pull request as ready for review May 29, 2023 09:04
@MoChilia MoChilia temporarily deployed to Automation test May 29, 2023 09:07 — with GitHub Actions Inactive
@MoChilia MoChilia temporarily deployed to Automation test May 29, 2023 09:12 — with GitHub Actions Inactive
@MoChilia MoChilia closed this Jun 12, 2023
@MoChilia
Copy link
Member Author

Closed this pr, it is included in #336.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants