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.

Sign up for GitHub

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

Jump to bottom

Azure Active Directory - External users #314

Closed
ReneHezser opened this issue Sep 11, 2020 · 23 comments · Fixed by #320
Closed

Azure Active Directory - External users #314

ReneHezser opened this issue Sep 11, 2020 · 23 comments · Fixed by #320
Labels
enhancement help wanted

Comments

@ReneHezser
Copy link

ReneHezser commented Sep 11, 2020 •
edited
Loading

AAD external users cannot be authenticated
An AAD user can authenticate, an external user within the same AAD is not able to autenticate.

I've identified the behavior being an empty username/UserPrincipalName of external users in AAD.

This is the log from vouch:

azure GetUserInfo: User: &{Username:[email protected] Name:René Hézser Email:[email protected] CreatedOn:0 LastUpdate:0 ID:0 TeamMemberships:[]}
verifyUser: Success! found user.Username in WhiteList: [email protected]"}

azure GetUserInfo: User: &{Username: Name:René Hézser (MS) Email:[email protected] CreatedOn:0 LastUpdate:0 ID:0 TeamMemberships:[]}
/auth Claims from userinfo: {Claims:map[email:[email protected]]}"}
/auth User is not authorized: verifyUser: user.Username not found in WhiteList:  . Please try again or seek support from your administrator"}

Using the Graph Explorer I can see that the userPrincipalName is null for an external user.
If I look at the code, would it make sense to check for an empty Username again after this line and set it from the email property?

Below is the result from Graph Explorer for 1) an AAD user and 2) an external account.

{
    "@odata.context": "https://graph.microsoft.com/v1.0/$metadata#users('---')/people",
    "@odata.nextLink": "https://graph.microsoft.com/v1.0/me/people/?$search=rene&$skip=0",
    "value": [
        {
            "id": "---'",
            "displayName": "René Hézser",
            "givenName": "René",
            "surname": "Hézser",
            "birthday": null,
            "personNotes": null,
            "isFavorite": false,
            "jobTitle": null,
            "companyName": null,
            "yomiCompany": null,
            "department": null,
            "officeLocation": null,
            "profession": null,
            "userPrincipalName": "[email protected]",
            "imAddress": "sip:[email protected]",
            "scoredEmailAddresses": [
                {
                    "address": "[email protected]",
                    "relevanceScore": 6,
                    "selectionLikelihood": "notSpecified"
                }
            ],
            "phones": [---],
            "personType": {
                "class": "Person",
                "subclass": "OrganizationUser"
            }
        },
        {
            "id": "---",
            "displayName": "[email protected]",
            "givenName": null,
            "surname": null,
            "birthday": null,
            "personNotes": null,
            "isFavorite": false,
            "jobTitle": null,
            "companyName": null,
            "yomiCompany": null,
            "department": null,
            "officeLocation": null,
            "profession": null,
            "userPrincipalName": null,
            "imAddress": null,
            "scoredEmailAddresses": [
                {
                    "address": "[email protected]",
                    "relevanceScore": -2,
                    "selectionLikelihood": "notSpecified"
                }
            ],
            "phones": [],
            "personType": {
                "class": "Person",
                "subclass": "ImplicitContact"
            }
        }
    ]
}

Some more information from azure GetUserInfo: getting user info from accessToken: 

{
    "aud": "00000003-0000-0000-c000-000000000000",
    "iss": "https://sts.windows.net/---/",
    "iat": 1599826218,
    "nbf": 1599826218,
    "exp": 1599830118,
    "acct": 1,
    "acr": "1",
    "aio": "AUQAu/---==",
    "altsecid": "1:live.com:---",
    "amr": [
        "pwd"
    ],
    "app_displayname": "Homeserver",
    "appid": "---",
    "appidacr": "1",
    "email": "[email protected]",
    "family_name": "Hézser",
    "given_name": "René",
    "idp": "live.com",
    "idtyp": "user",
    "ipaddr": "---",
    "name": "René Hézser (MS)",
    "oid": "---",
    "platf": "3",
    "puid": "---",
    "rh": "0.---.",
    "scp": "email openid profile",
    "signin_state": [
        "kmsi"
    ],
    "sub": "lfi------------",
    "tenant_region_scope": "EU",
    "tid": "---",
    "unique_name": "live.com#[email protected]",
    "uti": "---",
    "ver": "1.0",
    "xms_st": {
        "sub": "----10P14Fck"
    },
    "xms_tcdt": 1345116819
}
@bnfinet
Copy link
Member

bnfinet commented Oct 2, 2020

thanks @ReneHezser. Sorry for the delay in responding to your issue.

@simongottschlag @artagel do either of you have an opinion on this?

@ReneHezser just to be clear, you're suggesting plucking the email field for an external user, is that correct? Is there a clear indicator in the response that the user is an external user?

@simongottschlag
Copy link
Contributor

@ReneHezser @bnfinet if we change anything, it should be based on Azure AD v2 access token and not v1 - to future proof the solution. I think we get a claim called preferred_username in the v2 token which should be correct for both internal users and guests. (Important to remember that the value is mutable)

@ReneHezser can you change the AAD app to issue v2 tokens and verify that it looks correct with the guest?

@ReneHezser
Copy link
Author

Hi @simongottschlag. To be honest I need to check again, as I am not 100% certain it was not a configuration issue.
Next thing I try is to get the container running locally to do some debugging. (Never worked with golang before...)

But you should be able to quickly reproduce this by adding a live/hotmail/whatever account as external account to your AAD and use that user to login.

@ReneHezser
Copy link
Author

The app registration should be set to V2 (from the manifest "accessTokenAcceptedVersion": 2,).

Here is my config.yaml:

oauth:
  provider: azure
  client_id: ...
  client_secret: ...
  auth_url: https://login.microsoftonline.com/.../oauth2/v2.0/authorize
  token_url: https://login.microsoftonline.com/.../oauth2/v2.0/token
  callback_url: https://login.........org/auth
  user_info_url: https://graph.microsoft.com/v1.0/me
  scopes:
    - openid
    - email
    - profile

With the user info url I am not sure what to put in. I also tried https://graph.microsoft.com/oidc/userinfo. Both results look the same in the log.
The "ver":"1.0" is always 1 and not 2 in the access token.

Hmm. I don't get it. Where does the UPN come from? I don't see it being set? Not in the above and not in structs.go
scratcheshead

@simongottschlag simongottschlag mentioned this issue Oct 5, 2020
Merged
@simongottschlag
Copy link
Contributor

Hi @bnfinet @ReneHezser,

I've created a PR to see if this helps you: #320

If you add oauth.azure_token = id_token and see if it works.

I'm really bad at Golang and programming overall, please excuse any rookie mistakes. :) Any changes to the PR are appreciated.

@ReneHezser
Copy link
Author

The PR is working. 🥳

{"level":"debug","ts":1601969781.0134058,"msg":"azure GetUserInfo: getting user info from token: {\"aud\":\"...\",\"iss\":\"https://login.microsoftonline.com/.../v2.0\",\"iat\":1601969480,\"nbf\":1601969480,\"exp\":1601973380,\"aio\":\"...\",\"email\":\"[email protected]\",\"idp\":\"https://sts.windows.net/.../\",\"name\":\"René Hézser (MS)\",\"oid\":\"...\",\"preferred_username\":\"[email protected]\",\"rh\":\"...\",\"sub\":\"...\",\"tid\":\"...\",\"upn\":\"rene.hezser_....com#EXT#@....onmicrosoft.com\",\"uti\":\"...\",\"ver\":\"2.0\"}"}
{"level":"info","ts":1601969781.0136652,"msg":"azure GetUserInfo: User: &{Username:rene.hezser_....com#EXT#@....onmicrosoft.com Name:René Hézser (MS) Email:[email protected] CreatedOn:0 LastUpdate:0 ID:0 TeamMemberships:[]}"}
{"level":"debug","ts":1601969781.0136998,"msg":"/auth Claims from userinfo: {Claims:map[email:[email protected]]}"}
{"level":"error","ts":1601969781.0137134,"msg":"/auth User is not authorized: verifyUser: user.Username not found in WhiteList: rene.hezser_....com#EXT#@....onmicrosoft.com . Please try again or seek support from your administrator"}

And when I add rene.hezser_....com#EXT#@....onmicrosoft.com to the whitelist, I can access the protected website.
I also tested a normal AAD user, and it still works.

@bnfinet
Copy link
Member

bnfinet commented Oct 6, 2020 via email

@xtra-be
Copy link

xtra-be commented Nov 23, 2020

any luck this can be merged to master?
or update the docker image for this

bnfinet added a commit to simongottschlag/vouch-proxy that referenced this issue Dec 7, 2020
@bnfinet
vouch#314 exit on bad config
d08a4f7
@bnfinet
Copy link
Member

bnfinet commented Dec 7, 2020

@xtra-be I'm looking at this now and would like to merge to master in the coming days. Would you be in a position to help test?

@xtra-be
Copy link

xtra-be commented Dec 7, 2020

yes happy to test

we have a use case with external guests where this is a blocker

bnfinet added a commit to simongottschlag/vouch-proxy that referenced this issue Dec 8, 2020
@bnfinet
vouch#314 exit on bad config
b205a1b
@bnfinet bnfinet mentioned this issue Dec 8, 2020
Open
@xtra-be
Copy link

xtra-be commented Dec 16, 2020

has this allready been commit to the main branch?

@bnfinet
Copy link
Member

bnfinet commented Dec 16, 2020 •
edited
Loading

@xtra-be no I was waiting for you to test the PR with the additional code. My apologies if that was not clear.

If you could supply a log that shows it working to a gist that works be very helpful.

@xtra-be
Copy link

xtra-be commented Dec 17, 2020

can you provide me with a build to test?
i'm not that good in compiling go code :-(

@bnfinet
Copy link
Member

bnfinet commented Dec 17, 2020 via email

@xtra-be
Copy link

xtra-be commented Dec 17, 2020

as i say not realy good at go

when i try to build i get following error:

./do.sh build
go.uber.org/multierr

go.uber.org/multierr

../go.uber.org/multierr/error.go:197:6: undefined: errors.As
../go.uber.org/multierr/error.go:210:6: undefined: errors.Is

@jdavid5815
Copy link

jdavid5815 commented Dec 18, 2020 •
edited by bnfinet
Loading

Hi,

I just performed a new build and tried the patch in our testing environment. Unfortunately, it seems to be completely broken:

{"level":"debug","ts":1608286784.6549444,"msg":"/validate"}
{"level":"error","ts":1608286784.65498,"msg":"no jwt found in request"}
{"level":"debug","ts":1608286784.6549952,"msg":"setting the cookie domain to ********************"}
{"level":"debug","ts":1608286784.6550062,"msg":"CaptureWriter.Write set w.StatusCode 401"}
{"level":"info","ts":1608286784.6550722,"msg":"|401|   112.41µs /validate","statusCode":401,"request":8,"latency":0.00011241,"avgLatency":0.000042599,"ipPort":"*.*.*.*:49560","method":"GET","host":"**************","path":"/validate","referer":""}
{"level":"debug","ts":1608286784.675128,"msg":"/login"}
{"level":"debug","ts":1608286784.675144,"msg":"setting the cookie domain to ****************"}
{"level":"debug","ts":1608286784.6751714,"msg":"session state set to z7o2DR80DxNh6ElVX7XF862aFDYRWPEU"}
{"level":"debug","ts":1608286784.6751928,"msg":"Login url param normalized to 'https://*******************/feelgood/fr'"}
{"level":"debug","ts":1608286784.675199,"msg":"session requestedURL set to https://****************/feelgood/fr"}
{"level":"debug","ts":1608286784.6752026,"msg":"Adding code challenge"}
{"level":"debug","ts":1608286784.6752245,"msg":"saving session with failcount 1"}
{"level":"debug","ts":1608286784.675285,"msg":"/login looking for callback_url matching "}
{"level":"debug","ts":1608286784.6752903,"msg":"/login callback_url set to https://**************/auth"}
{"level":"error","ts":1608286784.6753564,"msg":"http: panic serving *.*.*.*:49562: runtime error: invalid memory address or nil pointer dereference\ngoroutine 38 [running]:\nnet/http.(*conn).serve.func1(0xc00012d220)\n\t/usr/local/go/src/net/http/server.go:1800 +0x139\npanic(0x97a880, 0xe8b2a0)\n\t/usr/local/go/src/runtime/panic.go:975 +0x3e3\ngithub.com/vouch/vouch-proxy/handlers.oauthLoginURL(0xc000190600, 0x0, 0x0, 0xc000149bf0, 0xc000117340, 0x1, 0xaeb500, 0xc0000f9100, 0xc0000270d0, 0xc, ...)\n\t/go/src/github.com/vouch/vouch-proxy/handlers/login.go:257 +0x7c7\ngithub.com/vouch/vouch-proxy/handlers.LoginHandler(0xaebe80, 0xc0000f9ac0, 0xc000190600)\n\t/go/src/github.com/vouch/vouch-proxy/handlers/login.go:100 +0xa2d\nnet/http.HandlerFunc.ServeHTTP(0xa32b48, 0xaebe80, 0xc0000f9ac0, 0xc000190600)\n\t/usr/local/go/src/net/http/server.go:2041 +0x44\ngithub.com/vouch/vouch-proxy/pkg/timelog.TimeLog.func1(0xaec1c0, 0xc0000b9340, 0xc000190500)\n\t/go/src/github.com/vouch/vouch-proxy/pkg/timelog/timelog.go:47 +0x193\nnet/http.HandlerFunc.ServeHTTP(0xc0000f92c0, 0xaec1c0, 0xc0000b9340, 0xc000190500)\n\t/usr/local/go/src/net/http/server.go:2041 +0x44\ngithub.com/gorilla/mux.(*Router).ServeHTTP(0xc0000c0300, 0xaec1c0, 0xc0000b9340, 0xc000190300)\n\t/go/src/github.com/gorilla/mux/mux.go:210 +0xe2\nnet/http.serverHandler.ServeHTTP(0xc0000b8700, 0xaec1c0, 0xc0000b9340, 0xc000190300)\n\t/usr/local/go/src/net/http/server.go:2836 +0xa3\nnet/http.(*conn).serve(0xc00012d220, 0xaedb80, 0xc000117240)\n\t/usr/local/go/src/net/http/server.go:1924 +0x86c\ncreated by net/http.(*Server).Serve\n\t/usr/local/go/src/net/http/server.go:2962 +0x35c\n"}
{"level":"info","ts":1608286795.24709,"msg":"|200|   26.659µs /healthcheck","statusCode":200,"request":9,"latency":0.000026659,"avgLatency":0.000040828,"ipPort":"127.0.0.1:59184","method":"GET","host":"0.0.0.0:9090","path":"/healthcheck","referer":""}

@bnfinet
Copy link
Member

bnfinet commented Dec 18, 2020

@jdavid5815 thank you for testing the PR

With that log I can't rule out that this error (panic) is configuration related. login.go:100 is not within the scope of this PR.

Could you please upload config and logs (full round trip) to a gist in the manner described in the README.

Thanks again.

@bnfinet
Copy link
Member

bnfinet commented Dec 18, 2020

@xtra-be the README includes instructions on how to build VP. If those instructions do not work for you, could you please work to improve them

@bnfinet bnfinet mentioned this issue Dec 28, 2020
Closed
@xtra-be
Copy link

xtra-be commented Jan 12, 2021

hello @bnfinet
seems to be working on my private install

@bnfinet
Copy link
Member

bnfinet commented Jan 12, 2021

@xtra-be If you could please supply a log that shows it working to a gist that would be very helpful.

@bnfinet
Copy link
Member

bnfinet commented Jan 28, 2021

@ReneHezser @simongottschlag @jdavid5815

Would any of you be in a position to test this PR, and if there is an error could you please upload your config and log as per the README. I would like to merge this work but given the inconsistent results I'm not comfortable doing so quite yet.

Thanks to @simongottschlag for the code and all for the help and support of VP.

@arionl
Copy link

arionl commented Mar 4, 2021

Hello. Complete newbie here but I have a similar use case: AzureAD with both native and guest users. Not sure if any of this will be helpful, but here is some info about what guests vs. normal users look like in my AzureAD tenant. For guest users, they don't have a UPN in accessToken as described above. However, both a native and a guest user seems to have a field called unique_name for me (I'm using https://graph.microsoft.com/oidc/userinfo in my config) which matches each user's email address.

Here's what my two users look like (I didn't know what sensitive stuff to remove, so I removed everything that looked like a hash, encoding, or GUID value):

AzureAD user - shown as "arion@AZUREAD" for email address:

iss:https://sts.windows.net/REMOVED/,
iat:1614820728,
nbf:1614820728,
exp:1614824628,
acct:0,
acr:1,
acrs:[urn:user:registersecurityinfo,urn:microsoft:req1,urn:microsoft:req2,urn:microsoft:req3,c1,c2,c3,c4,c5,c6,c7,c8,c9,c10,c11,c12,c13,c14,c15,c16,c17,c18,c19,c20,c21,c22,c23,c24,c25],
aio:REMOVED,
amr:[pwd,mfa],
app_displayname:homeproxy,
appid:REMOVED,
appidacr:1,
family_name:REMOVED,
given_name:Arion,
idtyp:user,
ipaddr:REMOVED,
name:Arion-AZUREAD,
oid:REMOVED,
platf:5,
puid:10033FFFA40B4408,
rh:REMOVED,
scp:email openid profile,
signin_state:[kmsi],
sub:REMOVED,
tenant_region_scope:NA,
tid:REMOVED,
unique_name:arion@AZUREAD,
upn:arion@AZUREAD,
uti:REMOVED,
ver:1.0,
wids:[REMOVED],
xms_st:{sub:REMOVED},
xms_tcdt:1502821947

AzureAD guest - shown as "arion@GUEST" for email address:

iss:https://sts.windows.net/REMOVED/,
iat:1614821321,
nbf:1614821321,
exp:1614825221,
acct:1,
acr:1,
acrs:[urn:user:registersecurityinfo,urn:microsoft:req1,urn:microsoft:req2,urn:microsoft:req3,c1,c2,c3,c4,c5,c6,c7,c8,c9,c10,c11,c12,c13,c14,c15,c16,c17,c18,c19,c20,c21,c22,c23,c24,c25],
aio:REMOVED,
altsecid:5::10032000B137F87B,
amr:[pwd],
app_displayname:homeproxy,
appid:REMOVED,
appidacr:1,
email:arion@GUEST,
family_name:REMOVED,
given_name:Arion,
idp:https://sts.windows.net/REMOVED/,
idtyp:user,
ipaddr:REMOVED,
name:Arion-GUEST,
oid:REMOVED,
platf:5,
puid:10030000A45AEF9C,
rh:REMOVED,
scp:email openid profile,
sub:REMOVED,
tenant_region_scope:NA,
tid:REMOVED,
unique_name:arion@GUEST,
uti:REMOVED,
ver:1.0,
wids:[REMOVED],
xms_st:{sub:REMOVED},
xms_tcdt:1502821947

According to Microsoft's docs, unique_name isn't a good thing to key off of though: https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens. Interesting bits from that document:

  • unique_name: Provides a human readable value that identifies the subject of the token. This value is unique at any given point in time, but as emails and other identifiers can be reused, this value can reappear on other accounts, and should therefore be used only for display purposes. Only issued in v1.0 id_tokens.
  • email: The email claim is present by default for guest accounts that have an email address. Your app can request the email claim for managed users (those from the same tenant as the resource) using the email optional claim. On the v2.0 endpoint, your app can also request the email OpenID Connect scope - you don't need to request both the optional claim and the scope to get the claim. The email claim only supports addressable mail from the user's profile information.

@acm-073
Copy link

acm-073 commented May 4, 2021

Hello,

I just encountered the issue too, and it's high priority for me because all users in my AD are guest users.

I just built the above PR (simongottschlag@6d8b79a) and deployed it to our service kubernetes cluster, in conjunction with nginx-ingress it is working like a charm :-)

See config and log files here: https://gist.github.com/acm-073/c7d91bca67c882c1e22e2aa8b4499bc4

I hope this helps in getting this PR ready to merge... the feature is very much wanted on our side.

Thanks & best regards
Albrecht

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Enable Azure AD v2 tokens simongottschlag/vouch-proxy
7 participants
@bnfinet @arionl @xtra-be @ReneHezser @simongottschlag @jdavid5815 @acm-073