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

💥 Pad values following RFC5054 #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

💥 Pad values following RFC5054 #13

wants to merge 1 commit into from

Conversation

LinusU
Copy link
Owner

@LinusU LinusU commented May 23, 2018

Fixes #12

Migration guide:

While verifiers generated with an earlier version of this library will continue to work, the client and the server must either both be using the padding, or both not.

The recommended upgrade path is to have both versions running on your server, which gives you time to upgrade all the clients. This can be accomplished by depending on this library two times:

{
  // ...
  "dependencies": {
    // ...
    "secure-remote-password": "^0.4.0",
    "secure-remote-password-pre-rfc5054": "LinusU/secure-remote-password#pre-rfc5054"
  }
}

You will then have to implement another version of your endpoint, which uses the newer version.

const loginCtrl1 = require('./controller/login-v1')
const loginCtrl2 = require('./controller/login-v2')

app.post('/v1/login-sessions', loginCtrl1.initiate)
app.post('/v1/login-sessions/:sessionId/finalize', loginCtrl1.finalize)

app.post('/v2/login-sessions', loginCtrl2.initiate)
app.post('/v2/login-sessions/:sessionId/finalize', loginCtrl2.finalize)

The login-v1 controller should import the library as such:

require('secure-remote-password-pre-rfc5054/server')

The login-v2 controller should import the library as usual:

require('secure-remote-password/server')

Fixes #12

Migration guide:

While verifiers generated with an earlier version of this library will continue to work, the client and the server must either both be using the padding, or both not.

The recommended upgrade path is to have both versions running on your server, which gives you time to upgrade all the clients. This can be accomplished by depending on this library two times:

```js
{
  // ...
  "dependencies": {
    // ...
    "secure-remote-password": "^0.4.0",
    "secure-remote-password-pre-rfc5054": "LinusU/secure-remote-password#pre-rfc5054"
  }
}
```

You will then have to implement another version of your endpoint, which uses the newer version.

```js
const loginCtrl1 = require('./controller/login-v1')
const loginCtrl2 = require('./controller/login-v2')

app.post('/v1/login-sessions', loginCtrl1.initiate)
app.post('/v1/login-sessions/:sessionId/finalize', loginCtrl1.finalize)

app.post('/v2/login-sessions', loginCtrl2.initiate)
app.post('/v2/login-sessions/:sessionId/finalize', loginCtrl2.finalize)
```

The `login-v1` controller should import the library as such:

```js
require('secure-remote-password-pre-rfc5054/server')
```

The `login-v2` controller should import the library as usual:

```js
require('secure-remote-password/server')
```
@LinusU
Copy link
Owner Author

LinusU commented May 23, 2018

ping @yallie, would love your 👀 on this

@LinusU LinusU mentioned this pull request May 23, 2018
@yallie
Copy link
Contributor

yallie commented May 23, 2018

Looks great! 👍

Don't you want to add the RFC5054 test vectors as a unit test?

@LinusU
Copy link
Owner Author

LinusU commented May 23, 2018

Their test vectors are for the 1024-bit group while this library is currently hardcoded to the 2048-bit group.

We should probably/maybe add a way to select the group, but I also think it's really nice that a sensible one is picked for you and you don't really have to think about it 🤔

@yallie
Copy link
Contributor

yallie commented May 23, 2018

but I also think it's really nice that a sensible one is picked for you and you don't really have to think about it

Totally agree with that! :)

@dobesv
Copy link

dobesv commented Jul 4, 2019

Wow, I did not know you could depend on two different versions of a package at once.

@LinusU
Copy link
Owner Author

LinusU commented Feb 12, 2020

Sorry for not moving forward with this in a very long time. I would really like to figure out if we should pad HNxorHG or not 🤔

cocagne/pysrp#38

If anyone has any information to chime in with I would be very happy to hear it!

@dobesv
Copy link

dobesv commented Feb 12, 2020

Is there an authoritative source you can ask? The SRP authors, or the authors of the RFC ?

@LinusU
Copy link
Owner Author

LinusU commented Feb 13, 2020

I tried emailing the four original authors of RFC 5054. Two of the email addresses bounced directly, but I'll keep this thread updated if I get a response from the other two.

@BuckarooBanzay
Copy link

just fyi: the master branch of this package didn't work for me so i tried it with this PR, works well as a client to https://github.com/est31/csrp-gmp 👍

Also: why don't you leave the decision to use padding or not to the user of the library, as an option/config for example?

@TheBinaryGuy
Copy link

Hey @LinusU, any updates on this? Is it ok to pin the package version to this PR?

TheBinaryGuy added a commit to Hushify/secure-remote-password that referenced this pull request Jul 7, 2023
@dobesv
Copy link

dobesv commented Jul 7, 2023

Instead of migrating via different package versions, could it be a flag passed in?

Maybe at first the flag defaults to the current padding method, and recommend to people they explictly specify this as true or false. Then do a major revision update where the padding defaults to this RFC 5054 format going forward so that this is more compliant with other implementations by default?

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.

Padding of g
6 participants