search

AzureAD / microsoft-authentication-library-common-for-android

Common code used by both the Active Directory Authentication Library (ADAL) and the Microsoft Authentication Library (MSAL)
MIT License
41 stars 35 forks source link

Allow OneAuth to bypass redirect uri check via build config #2250

Closed rpdome closed 10 months ago

rpdome commented 10 months ago

If set, the Redirect URI validation in Android Broker will be bypassed.

This will allow MSALTestApp/OneAuthTestApp to impersonate as 1st party apps (i.e. Outlook, OneDrive, etc).

shahzaibj commented 10 months ago

Can you add more context on why OneAuth wants to do this? If we accidentally ship this then we get a security bug and that is a nightmare. How do you plan to prevent that?

Example: Some time ago (2 years ago I believe) we accidentally shipped with "trust debug broker" enabled in prod.

rpdome commented 10 months ago

Can you add more context on why OneAuth wants to do this? If we accidentally ship this then we get a security bug and that is a nightmare. How do you plan to prevent that?

Example: Some time ago (2 years ago I believe) we accidentally shipped with "trust debug broker" enabled in prod.

They want to be able to test as a first party app.

As a precaution, I could

  1. add a test to fail if this is turned on.
  2. Skip if the version is in the MAJOR.MINOR.PATCH (all int) format.

Longer term, we could add an explicit step in the release pipeline to make sure all the settings that should not be turned on are not turned on in the final builds (e.g. this one, local flight settings, other build variables)

fadidurah commented 10 months ago
  1. add a test to fail if this is turned on.
  2. Skip if the version is in the MAJOR.MINOR.PATCH (all int) format.

Longer term, we could add an explicit step in the release pipeline to make sure all the settings that should not be turned on are not turned on in the final builds (e.g. this one, local flight settings, other build variables)

I implemented a similar check in my recent PR for the monthly release pipeline, where we fail if local flights are turned on, we can add additional conditions in that to also check for flags being passed. Although, in your pipeline PR, you only added this to the daily pipeline, not the release pipeline.

I think adding this sort of check is a necessity considering the nature of the change. We'll also need to add the check as part of the daily pipeline I think.

shahzaibj commented 10 months ago

Can you add more context on why OneAuth wants to do this? If we accidentally ship this then we get a security bug and that is a nightmare. How do you plan to prevent that? Example: Some time ago (2 years ago I believe) we accidentally shipped with "trust debug broker" enabled in prod.

They want to be able to test as a first party app.

  • The NAA flow I removed is one example.
  • The flow i'm working on (exposing prt account to foci apps) is another example.
  • They also said "We can't repro stuff like particular client_id's misbehaving/interacting with scopes in weird ways in brokered scenarios without that"

As a precaution, I could

  1. add a test to fail if this is turned on.
  2. Skip if the version is in the MAJOR.MINOR.PATCH (all int) format.

Longer term, we could add an explicit step in the release pipeline to make sure all the settings that should not be turned on are not turned on in the final builds (e.g. this one, local flight settings, other build variables)

Let's add the test to fail if this is turned on in Release builds.

Regarding "Skip if the version is in the MAJOR.MINOR.PATCH (all int) format." - I don't think we should do this. Often times when we build locally then the version is of that format and we might want this enabled on local builds. So I think we should just add tests and provide proper precautions in release pipeline.