Azure / az-hop

The Azure HPC On-Demand Platform provides an HPC Cluster Ready solution
https://azure.github.io/az-hop/
MIT License
65 stars 54 forks source link

Switch to `unique_name` instead of `upn` as the OIDC claim #1776

Closed ltalirz closed 9 months ago

ltalirz commented 10 months ago

In what area(s)?

/area administration /area ansible /area autoscaling /area configuration /area cyclecloud /area documentation /area image /area job-scheduling /area monitoring /area ood /area remote-visualization /area user-management

Describe the feature

For single sign-on via AAD, by default the config file suggests to use upn https://github.com/Azure/az-hop/blob/fb67753a8b44dfb3c9b95b7b1d40a7e628866501/config.tpl.yml#L360

However, with AAD apps the upn claim only exists for regular organizational accounts, not for guest users in the tenant (I can confirm this).

I.e. if you want guests of the tenant to be able to authenticate, the default choice should move away from upn (and even if you don't want guests to be able to log in, the current error message is not very helpful and you better do this by disabling guest access for the app at the Azure level).

For us, the best solution was to switch to unique_name, which is readable. Indeed, unique_name is a required claim, while upn is not.

If those turn out complicated in some cases, as in the linked post, either the username script needs to extract the right portion of the unique_name field, or you need to switch to random alphanumeric identifiers (and unix usernames) like oid or sub (example).

P.S. We first picked the email field, but it turns out this field was present only for guest users, but not for regular users.

xpillons commented 10 months ago

I think this need to be simply documented as each OpenID Connect implementation could be different. If you have a running implementation, please feel free to create a PR that I can review and test.

ltalirz commented 9 months ago

fixed in fa0e899c1976d6fe75e3390bd579911cb7d40879