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

Request to keep MFA feature #2196

Closed
jaisankar1 opened this issue Feb 9, 2023 · 7 comments
Closed

Request to keep MFA feature #2196

jaisankar1 opened this issue Feb 9, 2023 · 7 comments
Labels
clarification needed The issue is not accepted but we need clarification

Comments

@jaisankar1
Copy link

What version of UAA are you running? 75.22.2

Description of the issue:

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/184443444

The labels on this github issue will be updated when the story is started.

@strehle
Copy link
Member

strehle commented Feb 10, 2023

Hello @jaisankar1 ,

Some background first:
This was deprecated because of limited resources we have for maintenance of UAA. The feature itself was developed at a time where LDAP was mainly used and sometimes SAML for delegating the authentication. MFA is a requirement, yes, but we see more and more the usage of UAA as authentication proxy and MFA is active in the IdP. This means we see less use of this feature in UAA but MFA is active at the end of the delegated authentication, e.g. Azure AD, SAP IAM, Okta, etc...

MFA itself is not removed yet and if we do it, there will be a 77.+ release, so I strongly recommend that you upgrade to latest 76.x.x version. Release 76.x was created because of CSP and some breaking changes there. We will release 76.6.0 soon where CSP gets a configuration so that the breaking changes should not block you anymore.

@Tallicia @bruce-ricard any thoughts in addition?

@strehle strehle added the clarification needed The issue is not accepted but we need clarification label Feb 10, 2023
@Lax77
Copy link

Lax77 commented Feb 13, 2023

@strehle Could you let us know timeline for 77.+ release? This will help us better plan

@strehle
Copy link
Member

strehle commented Feb 16, 2023

@Lax77 there is no timeline and no final date for removal of MFA.

@Tallicia
Copy link
Contributor

@jaisankar1 Can you provide some examples on how this is being used and the protocols employed for why MFA is not being used with the IDP?

@swalchemist
Copy link
Contributor

Removing MFA is a priority for our team now and we're not sure yet if it will be in version 77 or 78.

peterhaochen47 added a commit that referenced this issue Feb 6, 2024
- Context about its deprecation:
  - This feature is under-utilized, and requires further
    maintenance for which our team lacks the resource. (For
    example, this feature is potentially vulnerable because
    a secure Content-Security-Policy cannot be applied to its
    pages without breaking them.) The feature has also been
    marked as "not ready for production" for a few years now.
    So we opt to remove the feature and instead recommend
    using the external IDPs's own MFA features. See more context
    in #2196.
- This commit removes all MFA-specific codes, except for
  the following, on which we will make follow-up commits:
 - README's deprecation notice
 - database operations
 - Content-Security-Policy's exemption toward MFA endpoint (https://github.com/cloudfoundry/uaa/blob/72565fb56cd1f90af499119d32c891937f3c5a76/server/src/main/java/org/cloudfoundry/identity/uaa/security/web/ContentSecurityPolicyFilter.java#L29)
- breaking changes planning: cloudfoundry/uaa-release#739
- Further notes about specific changes in tests:
  - For PasscodeMockMvcTests.testLoginUsingPasscodeWithUnknownToken(), the assertion
    on response code is changed from 401 to 403. This is because 403 was the original
    asserted value before MFA was added (see: 92abee6).
    The 403 response also makes sense in the context of the test (authentication
    present but has insufficient access).

 [#186854489]
peterhaochen47 added a commit that referenced this issue Feb 7, 2024
- Context about its deprecation:
  - This feature is under-utilized, and requires further
    maintenance for which our team lacks the resource. (For
    example, this feature is potentially vulnerable because
    a secure Content-Security-Policy cannot be applied to its
    pages without breaking them.) The feature has also been
    marked as "not ready for production" for a few years now.
    So we opt to remove the feature and instead recommend
    using the external IDPs's own MFA features. See more context
    in #2196.
- This commit removes all MFA-specific codes, except for
  the following, on which we will make follow-up commits:
 - README's deprecation notice
 - database operations
 - Content-Security-Policy's exemption toward MFA endpoint (https://github.com/cloudfoundry/uaa/blob/72565fb56cd1f90af499119d32c891937f3c5a76/server/src/main/java/org/cloudfoundry/identity/uaa/security/web/ContentSecurityPolicyFilter.java#L29)
- breaking changes planning: cloudfoundry/uaa-release#739
- Further notes about specific changes in tests:
  - For PasscodeMockMvcTests.testLoginUsingPasscodeWithUnknownToken(), the assertion
    on response code is changed from 401 to 403. This is because 403 was the original
    asserted value before MFA was added (see: 92abee6).
    The 403 response also makes sense in the context of the test (authentication
    present but has insufficient access).

 [#186854489]
@peterhaochen47
Copy link
Member

peterhaochen47 commented Feb 8, 2024

The removal is scheduled to happen at the next UAA release (v77.0.0), which we will release soon.

To add to what @strehle said, here is more context about this feature removal (quoting the commit message about this removal):

  - This feature is under-utilized, and requires further
    maintenance for which our team lacks the resource. (For
    example, this feature is potentially vulnerable because
    a secure Content-Security-Policy cannot be applied to its
    pages without breaking them.) The feature has also been
    marked as "not ready for production" for a few years now.
    So we opt to remove the feature and instead recommend
    using the external IDPs's own MFA features.

Can you please let us know the process, implementation details in order to retain this feature in forked version for current and future releases permanently?

If you wish to keep this feature on your fork and maintain it, you could add back the code removed in #2717

Since it's not going to be in upstream and only in forked version going forward, would that cause security concerns or any specific library upgrade that we need to take care?

Please see the above context on the security concerns. The MFA feature does rely on some dependencies that need to be bumped if you want to maintain it in your fork; you can find them in #2717. Also as mentioned, this feature has been marked "not ready for production," but we don't have good context on what would make it production-ready aside from the "Content-Security-Policy" issue.

peterhaochen47 added a commit that referenced this issue Feb 8, 2024
- Context about its deprecation:
  - This feature is under-utilized, and requires further
    maintenance for which our team lacks the resource. (For
    example, this feature is potentially vulnerable because
    a secure Content-Security-Policy cannot be applied to its
    pages without breaking them.) The feature has also been
    marked as "not ready for production" for a few years now.
    So we opt to remove the feature and instead recommend
    using the external IDPs's own MFA features. See more context
    in #2196.
- This commit removes all MFA-specific codes, except for
  the following, on which we will make follow-up commits:
 - README's deprecation notice
 - database operations
 - Content-Security-Policy's exemption toward MFA endpoint (https://github.com/cloudfoundry/uaa/blob/72565fb56cd1f90af499119d32c891937f3c5a76/server/src/main/java/org/cloudfoundry/identity/uaa/security/web/ContentSecurityPolicyFilter.java#L29)
- breaking changes planning: cloudfoundry/uaa-release#739
- Further notes about specific changes in tests:
  - For PasscodeMockMvcTests.testLoginUsingPasscodeWithUnknownToken(), the assertion
    on response code is changed from 401 to 403. This is because 403 was the original
    asserted value before MFA was added (see: 92abee6).
    The 403 response also makes sense in the context of the test (authentication
    present but has insufficient access).

 [#186854489]
peterhaochen47 added a commit that referenced this issue Feb 8, 2024
- Context about its deprecation:
  - This feature is under-utilized, and requires further
    maintenance for which our team lacks the resource. (For
    example, this feature is potentially vulnerable because
    a secure Content-Security-Policy cannot be applied to its
    pages without breaking them.) The feature has also been
    marked as "not ready for production" for a few years now.
    So we opt to remove the feature and instead recommend
    using the external IDPs's own MFA features. See more context
    in #2196.
- This commit removes all MFA-specific codes, except for
  the following, on which we will make follow-up commits:
 - README's deprecation notice
 - database operations
 - Content-Security-Policy's exemption toward MFA endpoint (https://github.com/cloudfoundry/uaa/blob/72565fb56cd1f90af499119d32c891937f3c5a76/server/src/main/java/org/cloudfoundry/identity/uaa/security/web/ContentSecurityPolicyFilter.java#L29)
- breaking changes planning: cloudfoundry/uaa-release#739
- Further notes about specific changes in tests:
  - For PasscodeMockMvcTests.testLoginUsingPasscodeWithUnknownToken(), the assertion
    on response code is changed from 401 to 403. This is because 403 was the original
    asserted value before MFA was added (see: 92abee6).
    The 403 response also makes sense in the context of the test (authentication
    present but has insufficient access).

 [#186854489]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification needed The issue is not accepted but we need clarification
Projects
Development

No branches or pull requests

7 participants