[AzureAD] fix broken AzureAD integration

[christianmeyer]

I fixed the AzureAD integration. The refactored code should support native logins and ADFS federated logins. I tried to incorporate all existing edge cases, but could not test all of them.

[christianmeyer]

this might fix among others, issues #689 , #662 , #645 , #624 , #573 , ...

[christianmeyer]

As it might be a bit hard to follow the thought process behind the refactoring, I've reiterated the changes and created a separate branch to illustrate the way of this refactoring. I didn't want to push hundreds of minor commits, so only pushed a proper merge. Feel free to have a look at the details, if that helps to understand the huge change: https://github.com/christianmeyer/saml2aws/tree/fix_azuread_in_steps

[theemethod]

I tried your v2.34.0-patch.1. I no longer receive the error I was getting but now receive: invalid character 'h' looking for beginning of value

[christianmeyer]

I tried your v2.34.0-patch.1. I no longer receive the error I was getting but now receive: invalid character 'h' looking for beginning of value

@theemethod would you be able to give a bit more context, like an anonymized verbose run?

my run, with MFA and ADFS path currently looks like:

DEBU[0000] Running               command=list-roles
Using IdP Account default to access AzureAD https://account.activedirectory.windowsazure.com
DEBU[0000] Get credentials       helper=osxkeychain serverURL="https://account.activedirectory.windowsazure.com"
DEBU[0000] Get credentials       helper=osxkeychain user=***
To use saved password just hit enter.
? Username ***
? Password ***

DEBU[0004] building provider     command=list idpAccount="account {\n  AppID: ***\n  URL: https://account.activedirectory.windowsazure.com\n  Username: ***\n  Provider: AzureAD\n  MFA: Auto\n  SkipVerify: false\n  AmazonWebservicesURN: urn:amazon:webservices\n  SessionDuration: 3600\n  Profile: saml\n  RoleARN: \n  Region: \n}"
DEBU[0005] HTTP Req              URL="https://login.microsoftonline.com/common/GetCredentialType?mkt=en-US" http=client method=POST
DEBU[0005] HTTP Res              Status="200 OK" http=client
DEBU[0005] HTTP Req              URL="https://sts.***/adfs/ls/?client-request-id=***" http=client method=POST
DEBU[0006] HTTP Res              Status="200 OK" http=client
DEBU[0006] HTTP Req              URL="https://login.microsoftonline.com:443/login.srf" http=client method=POST
DEBU[0006] HTTP Res              Status="200 OK" http=client
DEBU[0006] HTTP Req              URL="https://login.microsoftonline.com:443/kmsi" http=client method=POST
DEBU[0006] HTTP Res              Status="200 OK" http=client
DEBU[0006] HTTP Req              URL="https://account.activedirectory.windowsazure.com/" http=client method=POST
DEBU[0008] HTTP Res              Status="200 OK" http=client
DEBU[0008] HTTP Req              URL="https://login.microsoftonline.com:443/common/DeviceAuthTls/reprocess" http=client method=POST
DEBU[0008] HTTP Res              Status="200 OK" http=client
DEBU[0008] HTTP Req              URL="https://login.microsoftonline.com/common/SAS/BeginAuth" http=client method=POST
DEBU[0009] HTTP Res              Status="200 OK" http=client
? Enter verification code ******
DEBU[0072] HTTP Req              URL="https://login.microsoftonline.com/common/SAS/EndAuth" http=client method=POST
DEBU[0072] HTTP Res              Status="200 OK" http=client
DEBU[0072] HTTP Req              URL="https://login.microsoftonline.com/common/SAS/ProcessAuth" http=client method=POST
DEBU[0073] HTTP Res              Status="200 OK" http=client
***

I got a similar run for a non ADFS flow and also without MFA .. and I remember a conditional run via VPN also went fine. The only issue we faced at our company was an error out of mixing tenant and app IDs.

[theemethod]

I got a similar run for a non ADFS flow and also without MFA .. and I remember a conditional run via VPN also went fine. The only issue we faced at our company was an error out of mixing tenant and app IDs.

@christianmeyer Sure. I'm using Windows 10. You can ignore the cache errors. I was trying to figure out how to get the raw output that is failing. Not sure if that is possible though.

C:\Users\<user>>saml2aws login --verbose
time="2022-03-30T07:40:26-07:00" level=debug msg=Running command=login
time="2022-03-30T07:40:26-07:00" level=debug msg="Check if creds exist." command=login
time="2022-03-30T07:40:26-07:00" level=debug msg=Expand name="C:\\Users\\<user>/.aws/credentials" pkg=awsconfig
time="2022-03-30T07:40:26-07:00" level=debug msg=resolveSymlink name="C:\\Users\\<user>\\.aws\\credentials" pkg=awsconfig
time="2022-03-30T07:40:26-07:00" level=debug msg=ensureConfigExists filename="C:\\Users\\<user>\\.aws\\credentials" pkg=awsconfig
Using IdP Account default to access AzureAD https://account.activedirectory.windowsazure.com
To use saved password just hit enter.
? Username (<user>@<domain>) <user>@<tenant>.onmicrosoft.com

? Username <user>@<tenant>.onmicrosoft.com
? Password *********

time="2022-03-30T07:40:41-07:00" level=debug msg="building provider" command=login idpAccount="account {\n  AppID: <AppId>\n  URL: https://account.activedirectory.windowsazure.com\n  Username: <user>@<tenant>.onmicrosoft.com\n  Provider: AzureAD\n  MFA: Auto\n  SkipVerify: false\n  AmazonWebservicesURN: urn:amazon:webservices\n  SessionDuration: 43200\n  Profile: saml\n  RoleARN: arn:aws:iam::<account>:role/Azure_SSO_Full_Access\n  Region: us-west-1\n}"
time="2022-03-30T07:40:41-07:00" level=debug msg="Could not extract a valid expiry time for the MFA tokenmissing Response element" Cache_file="C:\\Temp\\saml2aws.txt" IdpAccount=default pkg=samlcache
time="2022-03-30T07:40:41-07:00" level=debug msg="Cache is invalid" command=login
Authenticating as <user>@<tenant>.onmicrosoft.com ...
time="2022-03-30T07:40:42-07:00" level=debug msg="HTTP Req" URL="https://login.microsoftonline.com/common/GetCredentialType?mkt=en-US" http=client method=POST
time="2022-03-30T07:40:42-07:00" level=debug msg="HTTP Res" Status="200 OK" http=client
time="2022-03-30T07:40:42-07:00" level=debug msg="HTTP Req" URL="https://login.microsoftonline.com/common/login" http=client method=POST
time="2022-03-30T07:40:42-07:00" level=debug msg="HTTP Res" Status="200 OK" http=client
invalid character 'h' looking for beginning of value
login response unmarshal error
github.com/versent/saml2aws/v2/pkg/provider/aad.(*Client).processAuthentication
        github.com/versent/saml2aws/v2/pkg/provider/aad/aad.go:556
github.com/versent/saml2aws/v2/pkg/provider/aad.(*Client).Authenticate
        github.com/versent/saml2aws/v2/pkg/provider/aad/aad.go:322
github.com/versent/saml2aws/v2/cmd/saml2aws/commands.Login
        github.com/versent/saml2aws/v2/cmd/saml2aws/commands/login.go:105
main.main
        github.com/versent/saml2aws/cmd/saml2aws/main.go:188
runtime.main
        runtime/proc.go:255
runtime.goexit
        runtime/asm_amd64.s:1581
error authenticating
github.com/versent/saml2aws/v2/pkg/provider/aad.(*Client).Authenticate
        github.com/versent/saml2aws/v2/pkg/provider/aad/aad.go:324
github.com/versent/saml2aws/v2/cmd/saml2aws/commands.Login
        github.com/versent/saml2aws/v2/cmd/saml2aws/commands/login.go:105
main.main
        github.com/versent/saml2aws/cmd/saml2aws/main.go:188
runtime.main
        runtime/proc.go:255
runtime.goexit
        runtime/asm_amd64.s:1581
Error authenticating to IdP.
github.com/versent/saml2aws/v2/cmd/saml2aws/commands.Login
        github.com/versent/saml2aws/v2/cmd/saml2aws/commands/login.go:107
main.main
        github.com/versent/saml2aws/cmd/saml2aws/main.go:188
runtime.main
        runtime/proc.go:255
runtime.goexit
        runtime/asm_amd64.s:1581

[christianmeyer]

@christianmeyer Sure. I'm using Windows 10. You can ignore the cache errors. I was trying to figure out how to get the raw output that is failing. Not sure if that is possible though.

@theemethod I was able to reproduce the error output under some MFA circumstances and patched it (MS seems kinda creative with their interruptions in the flow). Can you check if my v2.34.0-patch.2 does fix your issue? ... and btw, the raw output, to some extend, can be enabled by setting DUMP_CONTENT=true as described in the README

[theemethod]

@christianmeyer Sure. I'm using Windows 10. You can ignore the cache errors. I was trying to figure out how to get the raw output that is failing. Not sure if that is possible though.

@theemethod I was able to reproduce the error output under some MFA circumstances and patched it (MS seems kinda creative with their interruptions in the flow). Can you check if my v2.34.0-patch.2 does fix your issue? ... and btw, the raw output, to some extend, can be enabled by setting DUMP_CONTENT=true as described in the README

Thanks for the update. Unfortunately, the error output seems to be the same. This is a new testing tenant so there are no CA policies and MFA is disabled. I was able to get the raw output. Would any of that be useful?

[christianmeyer]

Thanks for the update. Unfortunately, the error output seems to be the same. This is a new testing tenant so there are no CA policies and MFA is disabled. I was able to get the raw output. Would any of that be useful?

@theemethod I've tested it against a brand new AzureAD setup after your feedback and disabled all Security Defaults to get down to the bare minimum flow, and it still works. Can you first post an updated --verbose run here? Posting the full DUMP_CONTENT=true here, would eventually expose sensible information, even though it would be interesting to know where exactly it steps out. I mean, you could grab the code and add a few log.Printf("...") around the area where the verbose run exits - if that's an option for you. Otherwise feel free to drop me an email (see git log) ... but please make sure to obscure any passwords especially in the POST payloads

[theemethod]

Thanks for the update. Unfortunately, the error output seems to be the same. This is a new testing tenant so there are no CA policies and MFA is disabled. I was able to get the raw output. Would any of that be useful?

@theemethod I've tested it against a brand new AzureAD setup after your feedback and disabled all Security Defaults to get down to the bare minimum flow, and it still works. Can you first post an updated --verbose run here? Posting the full DUMP_CONTENT=true here, would eventually expose sensible information, even though it would be interesting to know where exactly it steps out. I mean, you could grab the code and add a few log.Printf("...") around the area where the verbose run exits - if that's an option for you. Otherwise feel free to drop me an email (see git log) ... but please make sure to obscure any passwords especially in the POST payloads

@christianmeyer So I tested in our production tenant and everything works. We are currently federated with ADFS and I also have an account configured for our staged rollout to password hash sync. Both accounts worked just fine. One difference is that our staging tenant uses our @*.onmicrosoft.com userprincipalname. I can't easily change this, at least not right now. I can come back to the staging tenant issue in a few days once I finish this production update.

Thank you for your assistance.

[christianmeyer]

Given the exceptions introduced as part of this discussions, I've refactored the code again, to isolate them in the thought to be somehow linear authentication flow. The AzureAD authentication flow varies a lot, dependent on the security settings within AzureAD thus can have KMSI interrupts at felt random spots, same goes for (non) MFA flows.

[deepend-dev]

Does this also solves issue with idp url error if the tenant is in other directory?

[blang]

Is there any update on merging this? Any support needed?

[christianmeyer]

I've resolved the merge conflicts against master, that should incorporate the changes out of bd8ae8b, 3f9221e, 7ba431a, bcd6a14 and c0ae40a

[codecov-commenter]

Codecov Report

Merging #795 (c377703) into master (75b4721) will increase coverage by 3.81%. The diff coverage is 66.41%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #795      +/-   ##
==========================================
+ Coverage   27.88%   31.70%   +3.81%     
==========================================
  Files          52       52              
  Lines        7379     7463      +84     
==========================================
+ Hits         2058     2366     +308     
+ Misses       5069     4787     -282     
- Partials      252      310      +58     

Flag	Coverage Δ
unittests	31.70% <66.41%> (+3.81%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/provider/aad/aad.go	59.11% <66.41%> (+59.11%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mapkon]

I will merge this tomorrow

[realtimericky]

Hi guys, when is the version bump (to v2.36.6) expected?

[mapkon]

Definitely this week. Bare with me as I clear out time to release (apologies for the delay).

On Mon, Mar 27, 2023 at 22:42, realtimerick @.***> wrote:

Hi guys, when is the version bump (to v2.36.6) expected?

— Reply to this email directly, view it on GitHub https://github.com/Versent/saml2aws/pull/795#issuecomment-1485057426, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGQV5PE57LDOB7XA3CKOHTW6GDK7ANCNFSM5RPM7Y3Q . You are receiving this because you modified the open/close state.Message ID: @.***>