Merging Smartcard CBA Feature into Dev

[melissaahn]

Smartcard CBA

This branch contains a feature that allows AAD account users to authenticate via USB using their YubiKey. This work is being done in order to help satisfy Executive Order 14028, "Improving the Nation's Cybersecurity" .

Summary

• Certificate Based Authentication (CBA) will be expanded to allow logging in with certificates (PIV/CAC) provisioned onto YubiKeys.
• Users can plug in the USB of their YubiKey into their Android device, and once they determine that they would like to authenticate via CBA (by clicking "Sign in with certificate"), they will be lead through a set of dialogs which guide the user through selecting a certificate for authentication.
  • NFC support will be implemented in the next quarter. See "Future Work" section farther down below.
• For more details, please see the design documentation for this feature.

Steps

Note: This private preview testing document has detailed steps and screenshots for testing with Authenticator and Outlook.

1. User can plug in their YubiKey while the AuthorizationFragment is hosting, which will prompt an Android dialog for permission to connect to the YubiKey.
2. Upon permission granted, when the user clicks "Sign in with certificate", a dialog is displayed that allows the user to pick from eligible PIV certificates store on the YubiKey.
   • Note: the YubiKey must be plugged in prior to clicking "Sign in with certificate" if the user wants to use their YubiKey to authenticate. Otherwise, the fallback is for the feature to look for user certificates located on the mobile device.
3. The user should select a certificate and click "Continue". The next dialog shows a textbox in which the user should enter their YubiKey PIN.
   • If the user enters an incorrect PIN, they will see an error message asking them to try again. If a user has 3 incorrect attempts in a row, they won't be able to attempt again until their key gets unlocked (most likely, the group that originally provisioned the certificates will be able to assist in unlocking).
4. Once they enter their correct PIN, the WebView should close, and the user should be authenticated.

Some error handling has been implemented to address situations such as:

• User enters an incorrect PIN.
  • An error message appears under the (cleared) text box informing the user that they entered an incorrect PIN.
• User enters, or has entered, an incorrect PIN 3 times in a row.
  • Whenever the user attempts to interact with the YubiKey (in this context), a dialog pops up informing them that they have made too many incorrect PIN attempts. The user will not be able to proceed with smartcard CBA.
• User unplugs YubiKey in the middle of the dialog flow (after the "Sign in with certificate" button is clicked).
  • The current authentication flow is cancelled and a dialog pops up to inform the user that they will need to replugin their YubiKey in order to try again.
• User has no PIV certificates on their YubiKey.
  • The current authentication flow is cancelled and a dialog pops up to inform the user that no PIV certificates were found on the YubiKey.
• An exception is thrown during dialog flow relating to the YubiKey connection.
  • The current authentication flow is cancelled and a dialog pops up with a generic error message. The exception is logged and emitted as an ErrorEvent.

Sub-Branches

Pull Request	Summary of Additions/Changes	Notes
Melissaahn/yubi kit override integration by melissaahn · Pull Request #1729 · AzureAD/microsoft-authentication-library-common-for-android (github.com)	- keyboard and keyboardHidden added to configChanges of applicable activities within manifest. - Override for yubico.yubikit.android in order to clear error messages regarding the min sdk of Yubikit (19) being higher than ours (16). - Basic use of YubiKit that shows detection of a YubiKey being plugged in. - Method added to WebviewAuthorizationFragment that stops the detection of YubiKey devices when the fragment is about to be destroyed.	- These changes were originally merged into dev but were later taken out to keep smartcard CBA work in its own feature branch until ready for GA.
Implementing Cert Picker and PIN Dialogs for Smartcard CBA by melissaahn · Pull Request #1751 · AzureAD/microsoft-authentication-library-common-for-android (github.com)	- xml layout files for cert picker, PIN dialog, and max attempts dialog created. - smartcard cert picker now pops up when "sign in with certificate" is clicked and YubiKey is already plugged in. - smartcard cert picker shows PIV certs on YubiKey and prompts user to choose. - PIN dialog prompts user to enter PIN. If incorrect, an error message tells the user to try again. - If the user has three incorrect PIN attempts, a max attempts error dialog will show and the user won't be able to authenticate until they get their YubiKey unlocked. - If YubiKey is unplugged in the middle of the smartcard CBA dialog flow, the request is cancelled and an error dialog is shown.	- In short, this PR started up the UI for smartcard CBA.
Smartcard CBA Completed Flow by melissaahn · Pull Request #1766 · AzureAD/microsoft-authentication-library-common-for-android (github.com)	- YubiKit bumped up to 2.1.0-alpha.1, which brings in some classes (PivProvider and PivPrivateKey are notable) that allow smartcard CBA to finish. - The complete smartcard CBA flow now works. This involves adding a PivProvider instance in the Java Security static list of security providers. - max attempts error dialog is altered to become a generic error dialog that takes in strings as parameters. - DialogHolder class is created to manage the current dialog showing. - Some more string resources added for error messages. - Very simple telemetry started.	- In short, this PR got smartcard CBA to actually work.
Improving Telemetry In CBA Flows by melissaahn · Pull Request #1805 · AzureAD/microsoft-authentication-library-common-for-android (github.com)	- Telemetry is set up to report the statuses of PivProvider being added/removed/present in the Security static list of security providers. - Telemetry is set up for when CBA in general either succeeds or fails after we call proceed on the cert request from our side. - ErrorEvents are emitted when an unexpected exception is thrown.	- I'm more than willing to refactor this code when needed.
Adding Localization to Common by melissaahn · Pull Request #1807 · AzureAD/microsoft-authentication-library-common-for-android (github.com)	- Localizes Common. - Small change made to PIN dialog layout to ensure EditText types from right to left for right-to-left languages.	- PR sent to Authenticator: Pull request 6298556: Auth Client: Temporarily Adding Strings for Localization - Repos (visualstudio.com) - A good amount of the files added in this PR are a result of localization.
Bump YubiKit version to public 2.1.0 release by melissaahn · Pull Request #1808 · AzureAD/microsoft-authentication-library-common-for-android (github.com)	- YubiKit versions bumped to 2.1.0, which is considered to be a stable release. No breaking changes were observed.	- Release YubiKit 2.1.0 · Yubico/yubikit-android (github.com)
Added more precautions when handling PIN by melissaahn · Pull Request #1815 · AzureAD/microsoft-authentication-library-common-for-android (github.com)	- Instances where PIN is saved as a string were removed. PIN is set right away to a char array. - The PIN char array is cleared as soon as it is no longer needed on the client side.	- This PR was created recently, so it is not approved and merged yet.

Breaking Changes

Since YubiKeys are recognized as external keyboards on Android, a configuration change is detected when they are plugged in and unplugged from the device. The default response to this change is to restart the activity, which is unnecessary, and even harmful, in many cases. A simple fix is to add the flags keyboard and keyboardHidden to the configChanges attribute for every relevant (could be hosting when YubiKey is active) activity within the manifest. This fix was done for our libraries, but other apps may need to implement this change as well.

I created PRs for Authenticator and Company Portal that address the breaking change. I'm engaging with engineers on both teams to help explain why these changes need to be made.

• Authenticator: Pull request 6551394: Adding YubiKit Library and Keyboard ConfigChanges for Smartcard CBA - Repos (visualstudio.com)
• Company Portal: Pull request 6584465: Updated AndroidManifest.xml configChanges for smartcard CBA - Repos (visualstudio.com)

The broker release engineer for next month must take a commit after these PRs get merged for the RC builds.

For other apps that use MSAL, this breaking change and fix should be noted in the monthly announcement and wherever else it may be relevant.

Known Issues

• In internal testing during private preview, a bug was found where the user was unable to see the soft keyboard when the YubiKey was plugged in. The issue occurred while testing with a tablet, and this behavior hasn't been reported by other testers thus far.
  • It's possible that "Show on screen keyboard" isn't checked under "Physical Keyboard" within settings. I'm waiting for the tester to check their settings.
• In my own testing, the connection between the YubiKey and mobile device seems to not occur at times despite the user giving permission to connect. This results in the "Sign in with certificate" link leading to on-device certificates. I wouldn't consider it an issue that should block GA, since it happens very infrequently (which makes it difficult to debug) and it could vary among hardware devices, but it's worth noting.

Testing

Manual tests for a success scenario and two common failure scenarios (incorrect PIN and unplugging mid-dialog) were added under the Cert Based Auth folder in the master monthly test plan. I'm curious if I should create tests for 1p apps as well (for example, installing Authenticator and testing with production Outlook).

There could be some opportunities for automation that I still need to investigate. While I do think it would be valuable to at least manually plug in a YubiKey as a part of testing, the dialog steps that come afterwards could potentially be automated with the help of Robolectric Shadows. This ShadowUsbManager class could be interesting.

Private preview is currently in progress (with an apk that consumes a non-production version of YubiKit), but other than some internal testing, it doesn't seem that we will get too much feedback before GA. Yubico also has a copy of this apk and had some smaller followup questions regarding if NFC was implemented (it wasn't), if the apk contained the production version of YubiKit (it contains 2.1.0-alpha, as the apk was generated before 2.1.0 was released), and why they were seeing a server error if they clicked on "Sign in with a certificate" without a plugged in YubiKey (defaults to on-device, so device probably didn't have any applicable user certs).

I've created a successful apk of Authenticator with the feature. I'm investigating a small issue with Company Portal (it can't find new classes written in common4j..... but I have been given a potential commandline statement that could fix this issue).

Future Work

• YubiKit SDK also offers methods that help connect to a YubiKey via NFC. Smartcard CBA with NFC is work that I expect to pick up next quarter, and it will require an altered design of the current CBA flow. For example, a dialog asking if the user wants to use NFC will probably need to appear in the case where no YubiKey is plugged in.
• In the future, smartcard CBA will need to be extended to browsers as well, but this will most likely require some confirmation of initial work from eSTS before the feature can be started on the client side.

[codecov]

Codecov Report

Merging #1814 (c48fc9e) into dev (f839cd2) will decrease coverage by 0.63%.
The diff coverage is 2.59%.

@@             Coverage Diff              @@
##                dev    #1814      +/-   ##
============================================
- Coverage     14.79%   14.15%   -0.64%     
- Complexity      306      307       +1     
============================================
  Files           163      168       +5     
  Lines          6934     7317     +383     
  Branches        685      707      +22     
============================================
+ Hits           1026     1036      +10     
- Misses         5754     6127     +373     
  Partials        154      154              

Impacted Files	Coverage Δ
...mon/internal/platform/AndroidDevicePopManager.java	0.00% <ø> (ø)
...providers/oauth2/WebViewAuthorizationFragment.java	0.00% <0.00%> (ø)
.../ui/webview/AzureActiveDirectoryWebViewClient.java	62.25% <0.00%> (-3.21%)	⬇️
...w/challengehandlers/SmartcardCertPickerDialog.java	0.00% <0.00%> (ø)
.../ui/webview/challengehandlers/SmartcardDialog.java	0.00% <0.00%> (ø)
...ebview/challengehandlers/SmartcardErrorDialog.java	0.00% <0.00%> (ø)
.../webview/challengehandlers/SmartcardPinDialog.java	0.00% <0.00%> (ø)
...llengehandlers/ClientCertAuthChallengeHandler.java	4.32% <3.06%> (-6.49%)	⬇️
...nal/ui/webview/challengehandlers/DialogHolder.java	13.33% <13.33%> (ø)

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

[rpdome]

left some suggestion - most can be done in a separate (smaller) PR.

[rpdome]

Since we're already introducing a breaking change... maybe we should just consider bumping minSDK of our projects to 19.

Let's bring this up during standup.

[rpdome]

nit: let's try to figure out how you would extract ALL the yubikey logic from this class into one of its own. (and the regular CBA into another one).

what kind of interface would you need, etc.

[rpdome]

We can do that in a separate PR, as this one is meant to be "merge back to dev as-is" only.

[rpdome]

(This will allow us to do an easy-rollback if anything goes wrong as well).

[melissaahn]

Had a chat with Brian regarding this as well. Creating an abstract class to handle the YubiKit logic for ClientCertAuthChallengeHandler will be the next PR I'll work on.

[rpdome]

nit: could potentially extract 488-508 into a separate function.

[melissaahn]

I think the lines have changed a bit since merging the PIN PR. Were you referring to all the PivProvider/Static List logic? (or were the lines 498-508)

[melissaahn]

Thanks @rpdome for the review and approval. I've made changes in the latest commit that address some of the smaller comments (final, NonNull, removing some unnecessary comments).
Below is a list of things that need to be considered for future PRs:

• Can we bump the min sdk to 19? Brought this up in the latest scrum and it seemed like there wasn't currently any opposition to this (maybe we can even bump it to 24 to be able to use more Java 8 features)
• Extract YubiKit method calls into a separate abstract class that ClientCertAuthChallengeHandler can call so that YubiKit and the handler can be decoupled. This will help with rollbacks, if YubiKit has breaking changes in the future, and testing. (I'm going to start working on this right away)
• Spots to emit telemetry should continually be considered,