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

Fix error generating gpg subkeys for nistp256 and ed25519 primary keys #483

Closed
wants to merge 1 commit into from

Conversation

dlitz
Copy link

@dlitz dlitz commented Aug 2, 2024

This error would occur whenever the agent received a HAVEKEY request for the primary key that has a TREZOR-generated subkey, if the primary key algorithm was nistp256 or ed25519.

This does not fix the AssertionError that occurs when using brainpoolP256r1 or secp256k1 primary keys.

The specific error backtrace addressed by this patch is:

Traceback (most recent call last):
  File ".../libagent/gpg/__init__.py", line 290, in run_agent_internal
    handler.handle(conn)
  File ".../libagent/gpg/agent.py", line 250, in handle
    handler(conn, args)
  File ".../libagent/gpg/agent.py", line 102, in <lambda>
    b'HAVEKEY': lambda conn, args: self.have_key(conn, *args),
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../libagent/gpg/agent.py", line 213, in have_key
    self.get_identity(keygrip=keygrip)
  File ".../libagent/util.py", line 231, in wrapper
    result = method(self, *args, **kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../libagent/gpg/agent.py", line 174, in get_identity
    assert pubkey.key_id() == pubkey_dict['key_id']
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

Results before and after this commit:

                  |  before this change   |  after this change    |
                  |    subkey algo:       |    subkey algo:       |
primary key algo  |  ed25519  | nist256p1 |  ed25519  | nist256p1 |
------------------+-----------+-----------+-----------+-----------+
ed25519           | FAILED(1) | FAILED(1) |    OK     |    OK     |
nistp256          | FAILED(1) | FAILED(1) |    OK     |    OK     |
secp256k1         | FAILED(2) | FAILED(2) | FAILED(2) | FAILED(2) |
brainpoolP256r1   | FAILED(2) | FAILED(2) | FAILED(2) | FAILED(2) |
dsa3072           |    OK     |   OK      |    OK     |    OK     |
rsa3072           |    OK     |   OK      |    OK     |    OK     |

(1) Fails on `assert pubkey.key_id() == pubkey_dict['key_id']`
    This patch fixes these.

(2) Fails on `assert oid in SUPPORTED_CURVES, util.hexlify(oid)`
    This patch doesn't fix these.

I used the following script for testing on a Trezor Model T:

#!/bin/bash
# Usage: run-tests-combined.sh
#        run-tests-combined.sh primary_key_algo subkey_algo
#
# When run with no argument, this script re-invokes itself multiple
# times with different combinations of algorithms and outputs the
# result to ./test-results-$UNIX_TIMESTAMP.log.
#
# Otherwise, this script runs a single test with the given algorithms.
set -euC

if [ "$#" -eq 0 ]; then
    logfile="./test-results-$(date +%s).log"
    echo "Writing to $logfile"
    > "$logfile"

    for pka in rsa3072 dsa3072 brainpoolP256r1 secp256k1 nistp256 ed25519; do
        for ska in nist256p1 ed25519; do
            if "$0" "$pka" "$ska"; then
                printf '(%s, %s): OK\n' "$pka" "$ska" >> "$logfile"
                printf '\033[1m(%s, %s): OK\033[m\n' "$pka" "$ska"
            else
                printf '(%s, %s): FAILED\n' "$pka" "$ska" >> "$logfile"
                printf '\033[1m(%s, %s): FAILED\033[m\n' "$pka" "$ska"
            fi
        done
    done

    echo ""
    echo "Test results:"
    cat "$logfile"

    exit 0
fi

# Individual test begins here

primary_key_algo="$1"
subkey_algo="$2"

killall gpg-agent trezor-gpg-agent || true

userid='Test User <[email protected]>'
export GNUPGHOME=$(mktemp --tmpdir -d "trezor-gpg.XXXXXXXXXX" )
echo "GNUPGHOME is $GNUPGHOME"

# Generate primary key locally
gpg --batch --passphrase '' \
    --quick-gen-key "$userid" "$primary_key_algo" cert

# Generate subkey on Trezor
trezor-gpg init --homedir "$GNUPGHOME/trezor" \
    -e "$subkey_algo" -t "$(date +%s)" --subkey "$userid"

This error would occur whenever the agent received a HAVEKEY request for
the primary key that has a TREZOR-generated subkey, if the primary key
algorithm was nistp256 or ed25519.

This does not fix the AssertionError that occurs when using
brainpoolP256r1 or secp256k1 primary keys.

The specific error backtrace addressed by this patch is:

    Traceback (most recent call last):
      File ".../libagent/gpg/__init__.py", line 290, in run_agent_internal
        handler.handle(conn)
      File ".../libagent/gpg/agent.py", line 250, in handle
        handler(conn, args)
      File ".../libagent/gpg/agent.py", line 102, in <lambda>
        b'HAVEKEY': lambda conn, args: self.have_key(conn, *args),
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^
      File ".../libagent/gpg/agent.py", line 213, in have_key
        self.get_identity(keygrip=keygrip)
      File ".../libagent/util.py", line 231, in wrapper
        result = method(self, *args, **kwargs)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File ".../libagent/gpg/agent.py", line 174, in get_identity
        assert pubkey.key_id() == pubkey_dict['key_id']
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    AssertionError

Results before and after this commit:

                      |  before this change   |  after this change    |
                      |    subkey algo:       |    subkey algo:       |
    primary key algo  |  ed25519  | nist256p1 |  ed25519  | nist256p1 |
    ------------------+-----------+-----------+-----------+-----------+
    ed25519           | FAILED(1) | FAILED(1) |    OK     |    OK     |
    nistp256          | FAILED(1) | FAILED(1) |    OK     |    OK     |
    secp256k1         | FAILED(2) | FAILED(2) | FAILED(2) | FAILED(2) |
    brainpoolP256r1   | FAILED(2) | FAILED(2) | FAILED(2) | FAILED(2) |
    dsa3072           |    OK     |   OK      |    OK     |    OK     |
    rsa3072           |    OK     |   OK      |    OK     |    OK     |

    (1) Fails on `assert pubkey.key_id() == pubkey_dict['key_id']`
        This patch fixes these.

    (2) Fails on `assert oid in SUPPORTED_CURVES, util.hexlify(oid)`
        This patch doesn't fix these.

I used the following script for testing on a Trezor Model T:

    #!/bin/bash
    # Usage: run-tests-combined.sh
    #        run-tests-combined.sh primary_key_algo subkey_algo
    #
    # When run with no argument, this script re-invokes itself multiple
    # times with different combinations of algorithms and outputs the
    # result to ./test-results-$UNIX_TIMESTAMP.log.
    #
    # Otherwise, this script runs a single test with the given algorithms.
    set -euC

    if [ "$#" -eq 0 ]; then
        logfile="./test-results-$(date +%s).log"
        echo "Writing to $logfile"
        > "$logfile"

        for pka in rsa3072 dsa3072 brainpoolP256r1 secp256k1 nistp256 ed25519; do
            for ska in nist256p1 ed25519; do
                if "$0" "$pka" "$ska"; then
                    printf '(%s, %s): OK\n' "$pka" "$ska" >> "$logfile"
                    printf '\033[1m(%s, %s): OK\033[m\n' "$pka" "$ska"
                else
                    printf '(%s, %s): FAILED\n' "$pka" "$ska" >> "$logfile"
                    printf '\033[1m(%s, %s): FAILED\033[m\n' "$pka" "$ska"
                fi
            done
        done

        echo ""
        echo "Test results:"
        cat "$logfile"

        exit 0
    fi

    # Individual test begins here

    primary_key_algo="$1"
    subkey_algo="$2"

    killall gpg-agent trezor-gpg-agent || true

    userid='Test User <[email protected]>'
    export GNUPGHOME=$(mktemp --tmpdir -d "trezor-gpg.XXXXXXXXXX" )
    echo "GNUPGHOME is $GNUPGHOME"

    # Generate primary key locally
    gpg --batch --passphrase '' \
        --quick-gen-key "$userid" "$primary_key_algo" cert

    # Generate subkey on Trezor
    trezor-gpg init --homedir "$GNUPGHOME/trezor" \
        -e "$subkey_algo" -t "$(date +%s)" --subkey "$userid"
@dlitz
Copy link
Author

dlitz commented Aug 2, 2024

Actually, this looks like a duplicate of #470. Closing this.

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.

1 participant