-
Notifications
You must be signed in to change notification settings - Fork 290
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
Added OIDC support for login action #147
Added OIDC support for login action #147
Conversation
src/main.ts
Outdated
//Check for the credentials in individual parameters in the workflow. | ||
var servicePrincipalId = core.getInput('client-id', { required: false });; | ||
var servicePrincipalKey = null; | ||
var tenantId = core.getInput('tenant-id', { required: false }); | ||
var subscriptionId = core.getInput('subscription-id', { required: false }); | ||
var resourceManagerEndpointUrl = "https://management.azure.com/"; | ||
var enableOIDC= true; | ||
// If any of the individual credentials (clent_id, tenat_id, subscription_id) are missing | ||
if(!servicePrincipalId || !tenantId || !(subscriptionId || allowNoSubscriptionsLogin)){ | ||
//If all of the individual credentials (clent_id, tenat_id, subscription_id) are missing in workflow inputs, checking for creds object. | ||
if(!servicePrincipalId && !tenantId && (!subscriptionId || !allowNoSubscriptionsLogin)){ | ||
console.log('checking line 55 condition..') | ||
if(creds) { | ||
console.log('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); | ||
} | ||
else{ | ||
throw new Error("Credentials are not passed for Login action."); | ||
} | ||
} | ||
//If any few of the individual credentials are missing | ||
else | ||
throw new Error("Few credentials are missing.ClientId,tenantId are mandatory. SubscriptionId is also mandatory if allow-no-subscriptions is not set."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put it into a factory pattern. Your code will be much cleaner with no if/else conditions you have used below for enableOIDC check.
if (!servicePrincipalId || !servicePrincipalKey || !tenantId) { | ||
throw new Error("Not all values are present in the creds object. Ensure clientId, clientSecret and tenantId are supplied."); | ||
//Check for the credentials in individual parameters in the workflow. | ||
var servicePrincipalId = core.getInput('client-id', { required: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we name this var as clientId only to be same as Input name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is as per previous design only. clientID is treated as serviceprincipalId and clientSecret as serviceprincipalKey.
var servicePrincipalKey = null; | ||
var tenantId = core.getInput('tenant-id', { required: false }); | ||
var subscriptionId = core.getInput('subscription-id', { required: false }); | ||
var resourceManagerEndpointUrl = "https://management.azure.com/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. For other clouds it is different. For public cloud it is constant.
lib/main.js
Outdated
var subscriptionId = core.getInput('subscription-id', { required: false }); | ||
var resourceManagerEndpointUrl = "https://management.azure.com/"; | ||
var enableOIDC = true; | ||
// If any of the individual credentials (clent_id, tenat_id, subscription_id) are missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: please add newlines for formatting, readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
var servicePrincipalId = core.getInput('client-id', { required: false }); | ||
; | ||
var servicePrincipalKey = null; | ||
var tenantId = core.getInput('tenant-id', { required: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these also be individual secrets like in creds object?
src/main.ts
Outdated
execOptions.silent = !!silent; | ||
try { | ||
await exec.exec(`"${azPath}" ${command}`, args, execOptions); | ||
core.debug(args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't put log here, it may expose credentials if it doesn't get masked by GitHub runner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed 👍🏻
src/main.ts
Outdated
var args = []; | ||
if (enableOIDC) { | ||
args = [ | ||
"--allow-no-subscriptions", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create a base args array that contains common keys and based on the specific parameter values you can append to it.
Like if allowNoSubscriptionsLogin
is true -> append "--allow-no-subscriptions"
to args
, same for other keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!✅
if (enableAzPSSession) | ||
throw new Error(`Powershell login is not supported with OIDC.`); | ||
} else { | ||
throw new Error("Could not get ID token for authentication."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does OIDC send some error message in case of failure. if it is we should also show that to users, so user can debug the error. Here user can be clueless, what has gone wrong and he can't debug.
src/main.ts
Outdated
if (!servicePrincipalId || !tenantId || !(subscriptionId || allowNoSubscriptionsLogin)) { | ||
//If all of the individual credentials (clent_id, tenat_id, subscription_id) are missing in workflow inputs, checking for creds object. | ||
if (!servicePrincipalId && !tenantId && (!subscriptionId || !allowNoSubscriptionsLogin)) { | ||
if (creds) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the user has passed some parameters as inputs and some under creds object? As per your code, it will go for creds object which is ok. But Should we let users pass parameters to both?
var commonArgs = ["--service-principal", | ||
"-u", servicePrincipalId, | ||
"--tenant", tenantId | ||
]; | ||
if (allowNoSubscriptionsLogin) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (allowNoSubscriptionsLogin) { | |
if (allowNoSubscriptionsLogin) { | |
commonArgs = commonArgs.concat("--allow-no-subscriptions"); | |
} | |
if (enableOIDC) { | |
commonArgs = commonArgs.concat("--federated-token", idToken); | |
} | |
else { | |
commonArgs = commonArgs.concat("-p", servicePrincipalKey); | |
} | |
yield executeAzCliCommand(`login`, true, {}, commonArgs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this @BALAGA-GAYATRI
* Added OIDC support for login action (#147) * OIDC support for Powershell login (#156) * OIDC support ps (#155) ps login support * Update main.ts * formatting code * nit changes Co-authored-by: Balaga Gayatri <[email protected]>
No description provided.