Skip to content

Migration causing upgrade to fail (Specified key was too long) #107

Closed
#108
Closed
Migration causing upgrade to fail (Specified key was too long)#107
#108
Assignees
Potherca
Labels
Type: BugSomething isn't working
Milestone
v0.7.1

Description

@KDederichs
KDederichs
opened on Oct 18, 2022

Just FYI:

The migration that's part of 0.6.0 is causing updates to fail with:

Exception: Database error when running migration 000000Date20220923134100 for app solid An exception occurred while executing a query: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 3072 bytes

on MySql 10 databases with default config.

Activity

  1. JurgenG commented on Oct 18, 2022

    @JurgenG
    JurgenG
    on Oct 18, 2022

    I can confirm this. Having the same issue on our installation.

    System info:

    $ mysql -V
mysql  Ver 15.1 Distrib 10.5.15-MariaDB, for debian-linux-gnu (x86_64) using  EditLine wrapper
$ php -v
PHP 8.1.11 (cli) (built: Sep 29 2022 22:17:15) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.11, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.11, Copyright (c), by Zend Technologies
    with Xdebug v3.1.5, Copyright (c) 2002-2022, by Derick Rethans

    It looks like this has to do with the InnoDB limits:
    https://dev.mysql.com/doc/refman/5.7/en/innodb-limits.html
    https://mariadb.com/kb/en/innodb-system-variables/#innodb_large_prefix

  2. Potherca
    self-assigned this
    on Oct 19, 2022
  3. Potherca
    added
    Type: BugSomething isn't working
    on Oct 19, 2022
  4. Potherca
    added this to Solid Serverson Oct 19, 2022
  5. Potherca
    moved this to In Progress in Solid Serverson Oct 19, 2022
  6. Potherca
    added this to the milestone on Oct 19, 2022

  7. Potherca commented on Oct 19, 2022

    @Potherca
    Potherca
    on Oct 19, 2022
    Member

    Hi @KDederichs! Thank you for reporting (and @JurgenG for confirming) this issue.

    A quick search on StackOverflow gives similar reasons (InnoDB limits) as root cause. A lot of folks suggest using setting a string limit in the migration file using Schema::defaultStringLength(191);, but I am not yet fully convinced this is the best solution...

    I'll try to reprocude this locally and see which fix is most effective.

  8. Potherca
    modified the milestones: , v0.7.1 on Oct 19, 2022

  9. Potherca commented on Oct 19, 2022

    @Potherca
    Potherca
    on Oct 19, 2022
    Member

    (Oops, wrong milestone)

  10. JurgenG commented on Oct 19, 2022

    @JurgenG
    JurgenG
    on Oct 19, 2022

    Is there a reason we need key lengths of more than 3072 bytes?

  11. Potherca commented on Oct 20, 2022

    @Potherca
    Potherca
    on Oct 20, 2022
    Member

    The max length we need for the value is 2048 (for the URL / URI field1) but depending on charset collation (for instance UTF8 or UTF8-MB4) that can mean 8192 bytes are actually needed in order to store 2048 characters.

    I think the error is caused by the index on the uri filed: $table->addIndex(['jti', 'uri']); for which the length is longer than accepted for the index key2.

    I'm still thinking how much of the URL we actually need for the index... 🤔

    Footnotes

    1. Although the RFC/Spec does not define a maximum length, for historical reasons (namely Internet Explorer and the XML Sitemap Protocol) 2048 characters has become the convention. ↩

    2. But until I can trace where this is trigger by the DB Migration code I can't make an informed decision if the best solution is to set the collation in the PHP code, or to force the default string length, or something else entirely. ↩

  12. KDederichs commented on Oct 20, 2022

    @KDederichs
    KDederichs
    on Oct 20, 2022
    Author

    What's it used for?
    Is it maybe enough to just index the host part and not the query parameters?

  13. poef commented on Oct 20, 2022

    @poef
    poef
    on Oct 20, 2022
    Member

    According to the spec, the JTI claim must be unique for each request, except if a different URI is called. This means that we need to efficiently lookup if the given JTI + URI combination has already been used. In that case we must assume that someone is trying a replay attack. I'll try to find the exact quote in the spec.

  14. poef commented on Oct 20, 2022

    @poef
    poef
    on Oct 20, 2022
    Member

    https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop-11#section-11.1

    "To prevent multiple uses of the same DPoP proof servers can store, in the context of the target URI, the jti value of each DPoP proof for the time window in which the respective DPoP proof JWT would be accepted and decline HTTP requests to the same URI for which the jti value has been seen before. In order to guard against memory exhaustion attacks a server that is tracking jti values should reject DPoP proof JWTs with unnecessarily large jti values or store only a hash thereof."

  15. Potherca commented on Oct 20, 2022

    @Potherca
    Potherca
    on Oct 20, 2022
    Member

    As we need both the domain and the path, what would work is to hash the input and store that, rather than storing the actual URL. For instance using MD5 or SHA1, so we would only need 32 or 40 chars, respectively.

    This is something I have been thinking about anyway, for other reasons (security/privacy but also to have a fixed length db field).

    @poef Any opinions regarding that approach?

    Basically this: https://github.com/pdsinterop/solid-nextcloud/pull/108/files

  16. Potherca
    linked a pull request that will close this issueChange the JtiReplayDetector to use a hash rather than a full URL. #108on Oct 20, 2022

  17. poef commented on Oct 21, 2022

    @poef
    poef
    on Oct 21, 2022
    Member

    @Potherca Yes, that is an excellent solution!

  18. Potherca commented on Oct 21, 2022

    @Potherca
    Potherca
    on Oct 21, 2022
    Member

    Then I'll do some local testing and prepare a v0.7.1 release.

  19. VestigeJ commented on Nov 10, 2022

    @VestigeJ
    VestigeJ
    on Nov 10, 2022

    Just an additional note this is true on Snap installed Nextcloud trying to install 6.0 from the App store (Nextcloud Version 24.0.6)

  20. Potherca
    closed this as completedin #108on Nov 11, 2022
  21. Potherca
    added a commit that references this issue on Nov 11, 2022

    Merge pull request #108 from /issues/107-use-shorter-database-url-field

    6344e9c
  22. Repository owner moved this from In Progress to Done in Solid Serverson Nov 11, 2022

  23. Potherca commented on Nov 12, 2022

    @Potherca
    Potherca
    on Nov 12, 2022
    Member

    This issue should be resolved with release v0.7.1. Feedback welcome!

  24. KDederichs commented on Nov 12, 2022

    @KDederichs
    KDederichs
    on Nov 12, 2022
    Author

    I'll give it a try when it evenutally hits the nextcloud app store

  25. Potherca
    changed the title [-]Migration causing upgrade to fail[/-] [+]Migration causing upgrade to fail (Specified key was too long)[/+] on Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type: BugSomething isn't working

Type

No type

Projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions