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

Timestamp signatures from 0.x incompatible with 1.1 #120

Closed
jaraco opened this Issue Nov 12, 2018 · 10 comments

Comments

Projects
None yet
3 participants
@jaraco
Copy link
Contributor

jaraco commented Nov 12, 2018

Perhaps related to #115, we find that signatures produced on itsdangerous 0.24 are incompatible with 1.1. For example:

$ pip-run -q itsdangerous==0.24 -- -c "import itsdangerous; print(itsdangerous.Signer(b'secret-key').sign(b'my string').decode('ascii'))"
my string.wh6tMHxLgJqB6oY1uT73iMlyrOA
$ echo 'my string.wh6tMHxLgJqB6oY1uT73iMlyrOA' | pip-run -q itsdangerous==1.1 -- -c "import itsdangerous, sys; print(itsdangerous.Signer('secret-key').unsign(sys.stdin.read()))"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/pip-run-0f22xq6u/itsdangerous/signer.py", line 169, in unsign
    raise BadSignature("Signature %r does not match" % sig, payload=value)
itsdangerous.exc.BadSignature: Signature b'wh6tMHxLgJqB6oY1uT73iMlyrOA\n' does not match

Additionally, the engineer reports that

the expiration time is encoded and decoded differently [between versions]

This incompatibility has led our engineers to believe that it's necessary to upgrade all clients and producers simultaneously.

Is this incompatibility by design? Is there an approach that would allow the various signers/verifiers to use different versions of itsdangerous?

@davidism

This comment has been minimized.

Copy link
Member

davidism commented Nov 12, 2018

Looks like your example produced an extra newline due to echo. I had no issue signing a string with itsdangerous 0.24 and unsigning it with 1.1.0 when using the Python REPL.

Regarding the timestamps, see #46. You can override TimestampSigner.get_timestamp if you want to keep the previous offset behavior.

class EpochOffsetSigner(TimestampSigner):
    EPOCH = datetime(2011, 1, 1).timestamp()

    def get_timestamp(self):
        return int(time.time() - self.EPOCH)
@jaraco

This comment has been minimized.

Copy link
Contributor Author

jaraco commented Nov 12, 2018

Confirmed - the signer works fine on a roundtrip when I strip the newline:

$ pip-run -q itsdangerous==0.24 -- -c "import itsdangerous; print(itsdangerous.Signer(b'secret-key').sign(b'my string').decode('ascii'))" | pip-run -q itsdangerous==1.1 -- -c "import itsdangerous, sys; print(itsdangerous.Signer('secret-key').unsign(sys.stdin.read().strip()))"
b'my string'

Thanks for the update on the timestamps. Yes, I see the issue was reported in #118 also.

May I suggest that although the 1.0 bump does indicate some backward-incompatible changes, the changelog should be explicit about which backward-incompatible changes were made and (more importantly) what intervention the users of the library should do to account for the incompatible change? It was not clear to me reading the changelog that updating to 1.0/1.1 would break cross-application compatibility if timestamp signing was employed... and based on the effort in 1.1 to ensure fallback support for 0.24 w.r.t. algorithms, I'm surprised that a similar approach was not considered for timestamp signing.

Without fallback support in the library, I struggle with the transition plan for timestamp signing. In order migrate our apps from itsdangerous 0.x to 1.x, we'll still need to perform some sort of synchronized update of all of our apps... unless we configure every instance to use the workaround before upgrading. And even then, our apps would still be stuck on the old technique and would still require a synchronized update to remove the override.

Still, I'm grateful for the library and for the support. Please do consider my words above as simply conveying the frustration we've encountered for your edification and not as a criticism.

@jaraco

This comment has been minimized.

Copy link
Contributor Author

jaraco commented Nov 12, 2018

hmm. This is interesting - even using the timestamp signer, I'm unable to produce an error signing and unsigning across versions:

$ pip-run -q itsdangerous==0.24 -- -c "import itsdangerous; print(itsdangerous.TimestampSigner(b'secret-key').sign(b'my string').decode('ascii'))" | pip-run -q itsdangerous==1.1 -- -c "import itsdangerous, sys; print(itsdangerous.TimestampSigner('secret-key').unsign(sys.stdin.read().strip()))"
b'my string'
@davidism

This comment has been minimized.

Copy link
Member

davidism commented Nov 12, 2018

You'd need to pass max_age to unsign to trigger it.

For migrating, you should be able to do essentially what we did for the Serializer fallback. Keep two timestamp signers, one with the override, try the first, catch the exception and try the second. I'll leave it to @mitsuhiko whether he thinks that should warrant another release with more fallback code, rather than letting projects implement that themselves.

@jaraco

This comment has been minimized.

Copy link
Contributor Author

jaraco commented Nov 12, 2018

Confirmed - that replicates the failure:

$ pip-run -q itsdangerous==0.24 -- -c "import itsdangerous; print(itsdangerous.TimestampSigner(b'secret-key').sign(b'my string').decode('ascii'))" | pip-run -q itsdangerous==1.1 -- -c "import itsdangerous, sys; print(itsdangerous.TimestampSigner('secret-key').unsign(sys.stdin.read().strip(), max_age=5))"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/pip-run-ngnjxzzf/itsdangerous/timed.py", line 94, in unsign
    date_signed=self.timestamp_to_datetime(timestamp),
itsdangerous.exc.SignatureExpired: Signature age 1293840000 > 5 seconds

@jaraco jaraco changed the title Signatures from itsdangerous 0.x incompatible with 1.1 Timestamp signatures from 0.x incompatible with 1.1 Nov 12, 2018

@jaraco

This comment has been minimized.

Copy link
Contributor Author

jaraco commented Nov 29, 2018

I'll leave it to @mitsuhiko whether he thinks that should warrant another release with more fallback code, rather than letting projects implement that themselves.

Any opinion, @mitsuhiko?

@mitsuhiko

This comment has been minimized.

Copy link
Member

mitsuhiko commented Nov 29, 2018

I would not mind doing another release here. Not sure though if there is a reasonable compatibility path.

@jaraco

This comment has been minimized.

Copy link
Contributor Author

jaraco commented Jan 23, 2019

I've found this compatibility shim to work around the issue:

Don't edit: Don't use this shim; it's broken.

import datetime
import time

import itsdangerous


class EpochOffsetSigner(itsdangerous.TimestampSigner):
    EPOCH = datetime.datetime(2011, 1, 1).timestamp()

    def get_timestamp(self):
        return int(time.time() - self.EPOCH)


def unsign(signer, blob, **kwargs):
    try:
        return signer.unsign(blob, **kwargs)
    except itsdangerous.exc.SignatureExpired:
        compat_signer = EpochOffsetSigner(signer.secret_key)
        return compat_signer.unsign(blob, **kwargs)
@jaraco

This comment has been minimized.

Copy link
Contributor Author

jaraco commented Jan 23, 2019

As it turns out, the recipe above has a few flaws, which I worked through jaraco.crypto, culminating in this implementation and released as jaraco.crypto 2.1.

@jaraco

This comment has been minimized.

Copy link
Contributor Author

jaraco commented Jan 23, 2019

And here's an example showing it in action:

$ pip-run -q itsdangerous==0.24 -- -c "import itsdangerous; print(itsdangerous.TimestampSigner(b'secret-key').sign(b'my string').decode('ascii'))" | pip-run -q itsdangerous==1.1 jaraco.crypto==2.1 -- -c "import itsdangerous, sys; from jaraco.crypto.itsdangerous import compat; print(compat.unsign(itsdangerous.TimestampSigner('secret-key'), sys.stdin.read().strip(), max_age=5))"                                                                                                                                                                      
b'my string'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment