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

allow-no-subscriptions no longer working #371

Closed
kevinbasoftcat opened this issue Nov 20, 2023 · 15 comments
Closed

allow-no-subscriptions no longer working #371

kevinbasoftcat opened this issue Nov 20, 2023 · 15 comments
Assignees
Labels
bug Something isn't working

Comments

@kevinbasoftcat
Copy link

Have noticed today that the 'allow-no-subscriptions' parameter does not appear to be working. When running a workflow I am getting the below error even though the parameter is set to 'true.

Error: Login failed with Error: Not all parameters are provided in 'creds'. Double-check if all keys are defined in 'creds': 'clientId', 'clientSecret', 'subscriptionId', 'tenantId'.. Make sure 'az' is installed on the runner. If 'enable-AzPSSession' is true, make sure 'pwsh' is installed on the runner together with Azure PowerShell module. Double check if the 'auth-type' is correct. Refer to https://github.com/Azure/login#readme for more information.

@kevinbasoftcat kevinbasoftcat added the need-to-triage Requires investigation label Nov 20, 2023
@YanaXu
Copy link
Collaborator

YanaXu commented Nov 20, 2023

Hi @kevinbasoftcat ,

Sorry for the inconvenience. We have released a new version today.
If you want to keep the original behavior, please pin to the original version v1.4.6.

    - name: Login 
      uses: azure/login@v1.4.6
      with:
        creds: ${{secrets.AZURE_CREDENTIALS}}

And could you please provide your workflow file and the content of creds (you can only provide the keys in it).

@kevinbasoftcat
Copy link
Author

kevinbasoftcat commented Nov 20, 2023

Hi @YanaXu thanks for the quick response. I have switched to v1.4.6 and this is now working. In terms of my workflow I was using the below with the allow-no-subscriptions set to true. Will this option no longer be available in newer versions?

  • uses: Azure/login@v1
    with:
    creds: '{"clientId":"${{ secrets.AZURE_CLIENT_ID }}","clientSecret":"${{ secrets.AZURE_CLIENT_SECRET }}","tenantId":"${{ secrets.AZURE_TENANT_ID }}"}'
    allow-no-subscriptions: true

@YanaXu
Copy link
Collaborator

YanaXu commented Nov 20, 2023

Hi @kevinbasoftcat ,
Do you set secrets.AZURE_SUBSCRIPTION_ID in secrets? Is it an empty value?

@kevinbasoftcat
Copy link
Author

Hi @YanaXu, Apologies updated my comment I actually don't include the subscriptionID parameter at all I only include clientId, clientSecret and tenantId parameters in the creds section.

@YanaXu YanaXu pinned this issue Nov 20, 2023
@lra
Copy link

lra commented Nov 20, 2023

Same error for us.

We use:

      - uses: azure/login@v1
        with:
          creds: ${{ secrets.ARGO_AZ_DEV_SERVICE_ACCOUNT }}
          allow-no-subscriptions: true

with ARGO_AZ_DEV_SERVICE_ACCOUNT being:

{"clientId":"REDACTED","clientSecret":"REDACTED","tenantId":"REDACTED"}

Which leads to this error:

(node:1809) [DEP0128] DeprecationWarning: Invalid 'main' field in '/home/runner/work/_actions/azure/login/v1/node_modules/actions-secret-parser/package.json' of 'lib/index.js'. Please either fix that or report it to the module author
Error: Login failed with Error: Not all parameters are provided in 'creds'. Double-check if all keys are defined in 'creds': 'clientId', 'clientSecret', 'subscriptionId', 'tenantId'.. Make sure 'az' is installed on the runner. If 'enable-AzPSSession' is true, make sure 'pwsh' is installed on the runner together with Azure PowerShell module. Double check if the 'auth-type' is correct. Refer to https://github.com/Azure/login#readme for more information.
(Use `node --trace-deprecation ...` to show where the warning was created)

Is it a non-legit use anymore? Or is it a regression that's going to be fixed? Just want to know if we should stay on v1.4.6 until this is solved, or if we should change our code.

Thanks!

@YanaXu
Copy link
Collaborator

YanaXu commented Nov 20, 2023

Hi @lra , @kevinbasoftcat , sorry for your inconvenience. We'll provide a fix soon. Before that, please use v1.4.6.

On the other hand, we'd like to know why you have a configuration like this. Do you use a multi-tenant SP without any subscriptions at all? Could you explain more to help us understand? Because this leads to an insecure case which we do not want.

@lra
Copy link

lra commented Nov 20, 2023

We'll provide a fix soon. Before that, please use v1.4.6.

We'll do, thanks.

We inherited a complex setup of multiple AAD, subs and tenants, some with no subscription (pure AAD), and the same codebase needs to be usable from github action, locally, k8s, terraform, ... and from different actors (SP, humans).

This is our constraint.

To have the same codebase be usable on all our environments without lots of if/else cases to authenticate all those different clients, by only feeding 1 thing (in this case the Azure Service Account creds) and not changing any other parameter, we have to use the least constrained way to auth: without a subcription.

@evanrappe feel free to add more context here or correct me if I'm wrong as I never truly understood how this whole az auth logic really works.

@kevinbasoftcat
Copy link
Author

Hi @YanaXu we are an MSP and we have specific workflows that need to authenticate first then pull the list of subscriptions the customer has given us access to. We will not necessarily know a subscription ID in this scenario so have to pull that information after first authenticating.

@danelson
Copy link

@YanaXu would it be possible to point the v1 tag to v1.4.6 until a fix is delivered? That way users would have to explicitly upgrade to the breaking change.

@YanaXu
Copy link
Collaborator

YanaXu commented Nov 21, 2023

Hi @danelson and all, v1 is rollbacked to v1.4.6. Let's wait for the fix. Thanks for your patience.

@YanaXu
Copy link
Collaborator

YanaXu commented Dec 1, 2023

Hi @kevinbasoftcat , @johanols, @tomas-pajurek, @michal-zatloukal, @tokarchyn, @axelo, @evanrappe, @lra, @dimash-tr, and @syednaimamd, @danelson, we have released a new tag v1.5.1 today to fix this issue. Could you try v1.5.1 and tell us if it works fine? Thanks.

@evanrappe
Copy link

@YanaXu v1.5.1 looks good on our end. Thanks!

@danelson
Copy link

danelson commented Dec 1, 2023

@YanaXu v1.5.1 also looks good for me, thanks!

@kevinbasoftcat
Copy link
Author

@YanaXu v1.5.1 all working for me too thanks.

@YanaXu YanaXu added bug Something isn't working and removed need-to-triage Requires investigation labels Dec 5, 2023
@YanaXu
Copy link
Collaborator

YanaXu commented Dec 5, 2023

Hi all, we have moved tag v1 to v1.5.1. I'll close this issue. Thanks again for raising this issue and helping us test. Feel free to open new issues when you find any problems.

@YanaXu YanaXu closed this as completed Dec 5, 2023
@YanaXu YanaXu unpinned this issue Dec 5, 2023
YanaXu pushed a commit to YanaXu/loginFork that referenced this issue Dec 25, 2023
* fix371

* set subId once it is given

* modify test to logout first

* update tests

* update test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants