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

Improve AuthAdapter capabilities #7050

Closed
2 tasks done
Moumouls opened this issue Dec 8, 2020 · 33 comments
Closed
2 tasks done

Improve AuthAdapter capabilities #7050

Moumouls opened this issue Dec 8, 2020 · 33 comments
Labels
type:feature New feature or improvement of existing feature

Comments

@Moumouls
Copy link
Member

Moumouls commented Dec 8, 2020

Is your feature request related to a problem? Please describe.
No

Describe the solution you'd like
We need to add some triggers , and improve AuthAdapter interface to support some special auth systems like MFA, WebAuthn

Describe alternatives you've considered
No alternative currently, contributors need to inject code directly in Routers/Controllers may be we can avoid this.

Additional context
The new interface should support with AuthAdapter pure implementation:

  • MFA
  • WebAuthn

MFA particularities:

  • Store recovery keys somewhere (authData?)
  • perform a check after a login (could be a classic login or other loginWith adapter)
  • Need a public endpoint to receive/display recovery keys

@dblythy tell me if you see other things for MFA

WebAuthn:

  • Need 2 public endpoints (assertion/attestation)
  • Store multi public keys (can be done through authData)
@Moumouls Moumouls added the type:feature New feature or improvement of existing feature label Dec 8, 2020
@mtrezza
Copy link
Member

mtrezza commented Dec 8, 2020

I like this holistic approach, makes a lot of sense to me to standardize that interface.

@dblythy
Copy link
Member

dblythy commented Dec 8, 2020

I'd imagine the MFA adapter would need:

-SDK method to "enable" the authentication maybe Parse.User.enableAuth('mfa')
-SDK method to "verify" that the authentication is setup Parse.User.verifyAuth('mfa',data)
-SDK method to pass auth data to login (which can be accessed via the auth adapter Parse.User.logIn(username,password,{auth: authData})
-Secure storage of the secret used to verify MFA login (maybe an option in the adapter that strips out keys such as authData.mfa.secret)
-Secure storage and encryption of the recoveryKeys used to unlock the MFA

I think it is a good idea to make this approach available to other auth adapters, as I think it is likely that similar methods will be required for features such as passwordless auth.

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

Okay i think based on what i know currently of different auth protocal (OTP,MFA, WebAuthn)

We can add the challenge concept

Challenge can initiate the auth workflow (send sms, retreive challenge etc...)

then the current validateAuthData can take then the auth data can take over and resolve login/signup

@dblythy tell me if you think this way:

Note: OOS = Out of Scope

-SDK method to "enable" the authentication maybe Parse.User.enableAuth('mfa'): OOS it's a SDK issue
-SDK method to "verify" that the authentication is setup Parse.User.verifyAuth('mfa',data): OOS too
-SDK method to pass auth data to login (which can be accessed via the auth adapter Parse.User.logIn(username,password,{auth: authData}): OOS
-Secure storage of the secret used to verify MFA login (maybe an option in the adapter that strips out keys such as authData.mfa.secret): we can introduce an exclude system if authData is get from database to remove some sensitive informations from the response
-Secure storage and encryption of the recoveryKeys used to unlock the MFA, a secretFields key into AuthAdatper could do the job on server response to exclude this fields, may be a database projection is a better idea of implementation.

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

then the front process could be depending of the adapter used

// In MFA case, it will act as an activation is current user do not have MFA enabled
Pase.User.challenge("mfa")
// Then
Parse.User.loginWith('default,mfa', { username: "xxxx", password: "xxxxx", otp: "xxxx"})
// The user is logged in via default auth system and mfa 
// user will not be logged in if one of auth services fail

I think a string 'default,mfa' could be a nice feature, since it allow a front end developer to control the execution order to handle more easly erros, wrong credentials

the graphql mutation could look like

mutation challenge {
   challenge(input: {type: 'mfa'}){
      challenge
   }
}

GraphQL challenge type could be a opaque Scalar like ChallengeResponse

@dblythy
Copy link
Member

dblythy commented Dec 8, 2020

I don't think the MFA will be entirely OOS, as I think the token and the secret can be generated on the frontend, such as:

    const user = await Parse.User.signUp('username', 'password');
    const secret = otplib.authenticator.generateSecret();
    const token = otplib.authenticator.generate(secret); // this token would be generated from TPA app
    const recoveryKeys = await user._linkWith('mfa', { authData : {
      token,secret
}
    }); // secret stored attached to user.authdata and used to verify all login requests

Or, even do all the OTP work in the SDK when linkWith MFA is called, e.g:

    const user = await Parse.User.signUp('username', 'password');
    const recoveryKeys = await user._linkWith('mfa'); // all the otp work and UI happens in the SDK

The MFA adapter could validateAuthData using token and secret and return recovery codes, which the developer could then show in their own UI.

Next, perhaps we could introduce a method for validateLoginAuthData or something for Parse.User.logIn(username,password,{auth: mfa : {token}})

What are your thoughts on this approach? Would this work for WebAuthn?

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

based on

const myAuthData = {
  id: '12345678'  // Required field. Used to uniquely identify the linked account.
};
const user = new Parse.User();
await user.linkWith('providerName', { authData: myAuthData });

and http://parseplatform.org/Parse-SDK-JS/api/2.18.0/Parse.User.html#linkWith

Currently linkWith only return a Promise<void>, so we can return some data at this moment, with a special field on response rest operation like authDataResponse (here the current rest response https://docs.parseplatform.org/rest/guide/#signing-up-and-logging-in)

authDataResponse could be the validateAuthData return value.

The rest signup interface could be with username auth

curl -X POST \
  -H "X-Parse-Application-Id: ${APPLICATION_ID}" \
  -H "X-Parse-REST-API-Key: ${REST_API_KEY}" \
  -H "X-Parse-Revocable-Session: 1" \
  -H "Content-Type: application/json" \
  -d '{
        username: "test",
       password: "test",
        "authData": {
          "mfa": {
            // ID on authData should be optional now
            "secret": "xxxx";
            "token": "xxxx",
          }
        }
      }' \
  https://YOUR.PARSE-SERVER.HERE/parse/users

Signup with facebook and mfa could be

curl -X POST \
  -H "X-Parse-Application-Id: ${APPLICATION_ID}" \
  -H "X-Parse-REST-API-Key: ${REST_API_KEY}" \
  -H "X-Parse-Revocable-Session: 1" \
  -H "Content-Type: application/json" \
  -d '{
        "authData": {
          // On server authdata check, run in priority AuthData with ID, then add the user object to the context for next adapters
          "facebook": {
           id: "xxxxx"
           },
         // Then the mfa could be triggered after the user is created 
          "mfa": {
            // ID on authData should be optional now
            "secret": "xxxx";
            "token": "xxxx",
          }
        }
      }' \
  https://YOUR.PARSE-SERVER.HERE/parse/users

Then login with facebook looks exactly the same but the secret key will be ignored

curl -X POST \
  -H "X-Parse-Application-Id: ${APPLICATION_ID}" \
  -H "X-Parse-REST-API-Key: ${REST_API_KEY}" \
  -H "X-Parse-Revocable-Session: 1" \
  -H "Content-Type: application/json" \
  -d '{
        "authData": {
          // On server authdata check, run in priority AuthData with ID, then add the user object to the context for next adapters
          "facebook": {
           id: "xxxxx"
           },
         // Then the mfa could be triggered after the user is created 
          "mfa": {
            // ID on authData should be optional now
            "secret": "xxxx";
            "token": "xxxx",
          }
        }
      }' \
  https://YOUR.PARSE-SERVER.HERE/parse/users

The classic login could be

curl -X POST \
  -H "X-Parse-Application-Id: ${APPLICATION_ID}" \
  -H "X-Parse-REST-API-Key: ${REST_API_KEY}" \
  -H "X-Parse-Revocable-Session: 1" \
  -H "Content-Type: application/json" \
  -d '{
         // Parse server will try to validate first the username/password and then add the user object to the context
        // for the next mfa adapter
         username: "test",
         password: "test",
        "authData": {
         // Then the mfa could be triggered after the user is created 
          "mfa": {
            // ID on authData should be optional now
            "secret": "xxxx";
            "token": "xxxx",
          }
        }
      }' \
  https://YOUR.PARSE-SERVER.HERE/parse/users

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

@dblythy in MFA use does recovery keys come from the front or generated on the backend ?

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

the challenge endpoint could be (web auth example)

curl -X POST \
  -H "X-Parse-Application-Id: ${APPLICATION_ID}" \
  -H "X-Parse-REST-API-Key: ${REST_API_KEY}" \
  -H "X-Parse-Revocable-Session: 1" \
  -H "Content-Type: application/json" \
  -d '{
        "authData": {
          "webauthn": true  (could be also an object, the adapter need to validate it's own params structure)
        }
      }' \
  https://YOUR.PARSE-SERVER.HERE/parse/challenge

Example of challenge with pre logged user (SMS OTP example)

curl -X POST \
  -H "X-Parse-Application-Id: ${APPLICATION_ID}" \
  -H "X-Parse-REST-API-Key: ${REST_API_KEY}" \
  -H "X-Parse-Revocable-Session: 1" \
  -H "Content-Type: application/json" \
  -d '{
        "username": "test",
        "password": "test";
        "authData": {
          "otp": true  (could be also an object, the adapter need to validate it's own params structure)
        }
      }' \
  https://YOUR.PARSE-SERVER.HERE/parse/challenge

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

the GraphQL API could easly follow this pattern only need a challenge mutation.
The JS SDK implementation is also easy

// MFA
// One call for signup and login
const user = Parse.User.loginWith({
  username: "test"
  password: "test"
  authData: {
   mfa : { token, secret } // secret will be skipped if user already have a secret
 }
})

const { mfa: { recoveryKeys } } = user.get('authDataResponse')
// OTP
// User already created and have a phone field set
await Parse.User.challenge({
  username: "test"
  password: "test"
  authData: {
  otp: true
 }
})

const user = await Parse.User.loginWith({
  username: "test"
  password: "test"
  authData: {
  otp: token
 }
})
// WebAuthn for signup
await Parse.User.challenge({
  authData: {
  webauthn: true
 }
})

const user = await Parse.User.loginWith({
  authData: {
  webauthn: {
     id: credentialId
     data: webauthnData
  }
 }
})

Super secure auth could be

// WebAuthn for signup
await Parse.User.challenge({
 // need username and password since OTP need a pre logged user and id is not provided on webauthn
  username: "test",
  password: "test";
  authData: {
    webauthn: true // could be an object if adapter need some data
    otp: true // could be an object if adapter need some data
 }
})
// Parse server will check first the username/password, then webauthn, then otp and finally mfa
// if all checks are green use is logged in
const user = await Parse.User.loginWith({
  username: "test",
  password: "test" 
  authData: {
    webauthn: {
      id: credentialId
      data: webauthnData
    },
    otp: token
    mfa: { token }
 }
})
// Challenge with facebook pre login and otp
await Parse.User.challenge({
  authData: {
    facebook: {
      id: fbUserId
    }
    otp: true
 }
})

// Will pre log user with facebook and finish auth with otp validation
const user = await Parse.User.loginWith({
  authData: {
    facebook: {
      id: fbId
      ... otherFbInfos 
    },
    otp: token
 }
})

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

@dblythy what do you think about this examples ?

@dblythy
Copy link
Member

dblythy commented Dec 8, 2020

does recovery keys come from the front or generated on the backend ?

Currently backend, but It probably doesn't matter as long as they are long, secure strings that are stored as passwords.

I like the examples, I think i'll be able to get it going for MFA. I'm currently working on a quick proof of concept that i'll share with you in a minute to get your thoughts!

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

Since for companies where i work i use extensively AuthAdapter, i know exactly what we have to do to support the feature and at first glance the thing could be rather short to implement

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

Currently backend, but It probably doesn't matter as long as they are long, secure strings that are stored as passwords.

i think the recovery keys should be generated on the front end and only retrieved on set up time. The backend should only keep the validation secret ?

here the MFA example updated

// MFA
// One call for signup and login
const user = Parse.User.loginWith({
  username: "test"
  password: "test"
  authData: {
   mfa : { token, secret } // secret will be skipped if user already have a secret
 }
})

const { mfa: { recoveryKeys } } = user.get('authDataResponse')

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

we should use loginChallenge rest endpoint since challenge lacks of clarity and can conflicts with an already existing Challenge class

@Moumouls Moumouls linked a pull request Dec 8, 2020 that will close this issue
9 tasks
@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

@dblythy if you want to check, i started a draft PR

@dblythy
Copy link
Member

dblythy commented Dec 8, 2020

Great stuff @Moumouls. I wrote up a proof of concept of MFA (untested) working as an adapter instead of coded inbuilt. Feel free to use any of my existing code 😊. I created a new method 'loginWithAuthData', which can be used on any adapter to validate login requests. The adapter also handles all the encryption.

@Moumouls
Copy link
Member Author

Moumouls commented Dec 8, 2020

currently @dblythy i think that MFA do not need an additional endpoint here, the tricks for better handling

function validateAuthData(authData, { encryptionKey }, user) {
  // If user not defined it means that it's a signup
// so you can introduce loginWithAuthData here when user is not defined 
  if (!user || user.get('authData')) {
    throw new Parse.Error(
      Parse.Error.OBJECT_NOT_FOUND,
      'MFA auth is already enabled for this user.'
    );
  }

we will try to plug your Adapter with my draft PR once i finished some required stuff

Then a small JS SDK PR will be needed to support the new system we need to also update REST/GraphQL/Js SDK docs

@Taylorsuk
Copy link

This work looks great! I am looking to implement this soon, has there been any progress on it since the 8 Dec 20?

@Moumouls
Copy link
Member Author

Moumouls commented Jan 18, 2021

Hi @Taylorsuk ,

Yes the feature is finished, i currently use it with my forked package (waiting open source PRs to be merged). If you want to give it a try, you can install this package.

"parse-server": "moumouls/parse-server#beta.8" in your package.json

Here the webauthn example that use the new spec.

const verifyLogin = ({ assertion, signedChallenge }, options = {}, config, user) => {
  const dbAuthData = user && user.get('authData') && user.get('authData').webauthn;
  if (!dbAuthData)
    throw new Parse.Error(
      Parse.Error.OTHER_CAUSE,
      'webauthn not configured for this user or credential id not recognized.'
    );
  if (!assertion) throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'assertion is required.');
  const expectedChallenge = extractSignedChallenge(signedChallenge, config);
  try {
    const { verified, authenticatorInfo } = verifyAssertionResponse({
      credential: assertion,
      expectedChallenge,
      expectedOrigin: options.origin || getOrigin(config),
      expectedRPID: options.rpId || getOrigin(config),
      authenticator: {
        credentialID: dbAuthData.id,
        counter: dbAuthData.counter,
        publicKey: dbAuthData.publicKey,
      },
    });
    if (verified) {
      return {
        ...dbAuthData,
        counter: authenticatorInfo.counter,
      };
    }
    throw new Error();
  } catch (e) {
    throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'Invalid webauthn assertion');
  }
};

export const challenge = async (challengeData, authData, adapterConfig = {}, req) => {
  // Allow logged user to update/setUp webauthn
  if (req.auth.user && req.auth.user.id) {
    return registerOptions(req.auth.user, adapterConfig.options, req.config);
  }

  return loginOptions(req.config);
};

export const validateSetUp = async (authData, adapterConfig = {}, req) => {
  if (!req.auth.user)
    throw new Parse.Error(
      Parse.Error.OTHER_CAUSE,
      'Webauthn can only be configured on an already logged in user.'
    );
  return { save: await verifyRegister(authData, adapterConfig.options, req.config) };
};

export const validateUpdate = validateSetUp;

export const validateLogin = async (authData, adapterConfig = {}, req, user) => {
  if (!user) throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'User not found for webauthn login.');
  // Will save updated counter of the credential
  // and avoid cloned/bugged authenticators
  return { save: verifyLogin(authData, adapterConfig.options, req.config, user) };
};

export const policy = 'solo';

Then the MFA not already implemented will be a good example of and additional Auth Adapter (only requested once the user will have configured the MFA)

@Taylorsuk
Copy link

@Moumouls Amazing work! Thank you for the example code too. We'll start a feature branch with your fork and then by the time it's tested, hopefully things will be released in the core package. 🥳

@Moumouls Moumouls linked a pull request Jan 18, 2021 that will close this issue
7 tasks
@Moumouls
Copy link
Member Author

@Taylorsuk yes i also hope that PRs will be merged soon.
Also a little note here the challenge endpoint is not ready on the Parse JS SDK: parse-community/Parse-SDK-JS#1276

@dblythy
Copy link
Member

dblythy commented Sep 4, 2021

@mtrezza @Moumouls how is this issue going?

I've been thinking about it and I think this is almost a "secondary auth", rather than a traditional authentication adapter that allows users to login with external servers.

I think you should be able to set "secondaryAuth" adapters via server options, such as:

secondaryAuth : {
   mfa: {
     key: 'xxx'
   }
}

Then, when any other traditional auth method is used, a Parse Error is thrown such as 'AUTH REQUIRED'.

The Adapter could look like:

validateLogin(authData, user) {
  if (user.secondaryAuthData.mfaEnabled) {
    // check validity of provided secrets. Throw if incorrect/
  }
}
registerSecondaryAuth(user, secondaryAuth) {
  secondaryAuth.mfaEnabled = true
  // parse server then handles secure saving of secondaryAuth
}

Then, in the SDKS, it could be:

Parse.User.logIn('user', 'pass', {secondaryAuth:});
Parse.User.registerSecondaryAuth('mfa');

I think it would be cool to have secondary SMS auth, secondary OTPLib auth (#4024), secondary email auth, all in V5.0 of Parse Server.

@Moumouls
Copy link
Member Author

Moumouls commented Sep 4, 2021

Hi @dblythy I sent a PR to support multi auth system (primary, secondary).

The system is implemted here: #7079

In my team we currently use this code from now 8months in prod. We have easily integrated the SMS OTP auth adapter.

For parse I definitely need to find some open source time asap to help to finish Defined Schema and Auth Adapter rework with better DX and 2nd auth adapter support.

@dblythy feel free to take a look at the PR and tests. 🙂

PS: I hope to dedicate many days to parse mid/end September, for the V5 :)

@mtrezza
Copy link
Member

mtrezza commented Sep 4, 2021

It would be great if someone wanted to pick this PR up. There is not much time before release 5, so given the size of these 2 PRs, it think it would have to start now or it won't make it into the initial Parse Server 5.0. The feature freeze will begin around end of September / beginning of October. They can of course be releases during 2022 as optional features, if they can be merged without breaking change. Otherwise the next chance is in 2023 with release 6.

I suggest let's try to assemble a team who can dedicate some time and distribute the effort. The 2 auth PRs (and schema migration) are definitely priority PRs.

@dblythy
Copy link
Member

dblythy commented Sep 4, 2021

I would be happy to pick it up, I just haven’t wrapped my head around it and I’m not overly sure what has to change for it to be ready. I would like to get an MFA adapter for V5 too.

@mtrezza
Copy link
Member

mtrezza commented Sep 4, 2021

@Moumouls can these 2 auth PRs be merged individually or should they be treated as 1 PR?

In other words, if only this PR gets merged, but #7079 doesn't make it for time reasons, would that leave us with a buggy auth adapter or an auth adapter that needs to be again fundamentally remodeled in #7079?

And does any of the 2 PRs require a breaking change?

@Moumouls
Copy link
Member Author

Moumouls commented Sep 4, 2021

@mtrezza my 2 PRs (Schema & Auth) DO NOT introduce any breaking change.

My company run this 2 PR for now 9 month on prod :), from my point of view, they have reached a mature state.

[redacted by @mtrezza]

The 2 PR implements only some new optional features and could be released just after the V5.

Also I've a PR on JS SDK also to support the new auth challenge endpoint. Here we just need to write some doc I think and adjust some typo.

@mtrezza @dblythy if it's okay for you let me schedule some open source time in September, to dive again in this 2 PR (since our company is currently at the end of a long code security audit)

@mtrezza due to the huge PRs i totally agree about may be making 2 teams, i'll validate and may be open a slack discussion with @sadortun for the Defined PR and @dblythy if it's okay for you to collab on the Auth PR.

@dblythy feel free to make a quick PR review on the Auth PR, then we can open a slack discussion.

@dblythy @sadortun @mtrezza are you ok to a special slack channel to collab and finish the final stuff on this 2 PRs ? :)

@Moumouls
Copy link
Member Author

Moumouls commented Sep 4, 2021

I think we should make 2 distinct PR the Auth Rework and then the quick MFA. Auth PR is already huge (completely my fault).

But @dblythy could directly use my branch to start the MFA Adapter addition. (Super easy to implement with the new system, but we need some detailed doc on this, and all new Auth Adapter interface)

@mtrezza
Copy link
Member

mtrezza commented Sep 4, 2021

@Moumouls can these 2 auth PRs be merged individually or should they be treated as 1 PR?

In other words, if only this PR gets merged, but #7079 doesn't make it for time reasons, would that leave us with a buggy auth adapter or an auth adapter that needs to be again fundamentally remodeled in #7079?

@mtrezza
Copy link
Member

mtrezza commented Sep 4, 2021

Okay, so I take it that even if we merge only this auth PR, it would be a good step, if we can merge the 2nd one, even better, but it would be any technical issue if can can't for time reasons.

@dblythy Do you think creating a new branch make more sense, to keep the original one intact and as reference without modification?

@dblythy
Copy link
Member

dblythy commented Sep 4, 2021

How would that work? Would I fork #7079?

@mtrezza
Copy link
Member

mtrezza commented Sep 4, 2021

You could just create a new branch from where this PR currently is and open a PR for the new branch.

@kishanio
Copy link

I believe this is ready and merged. Can we close this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
5 participants