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

[Azure AD] Fixes #530: Supports KMSI page display logic after MFA #538

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

kenchan0130
Copy link
Contributor

@kenchan0130 kenchan0130 commented Aug 12, 2020

Background

I have previously added processing for KSMI pages after MFA at #509.
However, we have received reports from some users at #530 that authentication using Azure AD does not work properly.

Cause

I found the following description at https://docs.microsoft.com/en-us/azure/active-directory/fundamentals/keep-me-signed-in.

The following diagram shows the user sign-in flow for a managed tenant and federated tenant and the new keep me signed in prompt. This flow contains smart Logic so that the Stay signed in? option won't be displayed if the machine learning system detects a high-risk sign-in or a sign-in from a shared device.

This means that if the device is not trusted, the KMSI page may not be displayed.

Patch overview

  • Supports KMSI page display logic after MFA

FIX: #530

@@ -847,34 +847,35 @@ func (ac *Client) Authenticate(loginDetails *creds.LoginDetails) (string, error)
resBody, _ = ioutil.ReadAll(res.Body)
resBodyStr = string(resBody)

var ProcessAuthJson string
// After performing MFA we may be prompted with KMSI (Keep Me Signed In) page
// Ref: https://docs.microsoft.com/ja-jp/azure/active-directory/fundamentals/keep-me-signed-in
if strings.Contains(resBodyStr, "$Config") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the information in resBodyStr of #530 (comment) and resBodyStr of #509, I determine if it is a KSMI page based on the presence of $Config.

If this intent is difficult to convey, I could change it to the following.

isKMSIPage := strings.Contains(resBodyStr, "$Config")
if isKMSIPage {
  ....
}

@pseidel-kcf
Copy link
Contributor

@kenchan0130 Thanks for investigating my issue! I tested this branch locally and ran into an issue : error authenticating to IdP: unable to locate IDP oidc form submit URL

I believe this is due to res.Body already being consumed on line 847.

The diff below fixes the issue for me locally but I'm not a Go expert so I'm not sure if this is the best approach.

diff --git a/pkg/provider/aad/aad.go b/pkg/provider/aad/aad.go
index 68afcbe..5fe8d5e 100644
--- a/pkg/provider/aad/aad.go
+++ b/pkg/provider/aad/aad.go
@@ -1,6 +1,7 @@
 package aad

 import (
+       "bytes"
        "crypto/tls"
        "encoding/json"
        "fmt"
@@ -846,6 +847,8 @@ func (ac *Client) Authenticate(loginDetails *creds.LoginDetails) (string, error)
                // <script><![CDATA[  $Config=......; ]]>
                resBody, _ = ioutil.ReadAll(res.Body)
                resBodyStr = string(resBody)
+               // reset res.Body so it can be read again later if required
+               res.Body = ioutil.NopCloser(bytes.NewBuffer(resBody))

                // After performing MFA we may be prompted with KMSI (Keep Me Signed In) page
                // Ref: https://docs.microsoft.com/ja-jp/azure/active-directory/fundamentals/keep-me-signed-in

@kenchan0130
Copy link
Contributor Author

Indeed, io.Reader is treated like a stream. Therefore, I also noticed that it can't be read twice, which would affect the subsequent ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.26.2 AzureAD broken starting earlier in the week but 2.26.1 works
3 participants