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.

Sign up for GitHub

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

Jump to bottom

signer API: add public key attribute to Signer interface #605

Closed
lukpueh opened this issue Jul 4, 2023 · 4 comments · Fixed by #756
Closed

signer API: add public key attribute to Signer interface #605

lukpueh opened this issue Jul 4, 2023 · 4 comments · Fixed by #756
Labels
contribfest Issues for KubeCon EU contribfest good first issue

Comments

@lukpueh
Copy link
Member

lukpueh commented Jul 4, 2023

Currently, the Signer interface does not prescribe the internal data structure of a signer implementation. This is because a signer is responsible for one thing only -- signing, and can be completely decoupled from private key data, and mostly decoupled from public key data.

The latter actually is part of the interface method from_priv_key_uri, which is the canonical way of loading signers, and is usually based on information from the public key (e.g. keytype/scheme).

While signers don't necessarily require the full public key object once loaded, all current implementations 1 do cache and use it for signing, usually to dispatch on scheme, and assign keyid to the resulting signature.

If we add the de-facto interface field public_key: Key indeed to the interface, signers could also be used to verify their signatures, for which use cases exist 2.

The alternative to this proposed change would be to introduce a container format for "signer, public key"-tuples. Although, this seems unnecessary, if most signer already contain the public key.

Footnotes

  1. With the exception of SSlibSigner, which could easily provide an alias for public_key using the internal data structure, and might even become obsolete (see Add replacement API for '*keys' and 'interface' key generation and import  #604) ↩

  2. runlib.in_toto_record_stop needs to verify preliminary link metadata, and then re-sign it. ↩
@jku
Copy link
Collaborator

jku commented Jul 4, 2023

This makes sense to me.

This does mean that from_priv_key_uri() could have been a Signer constructor (and caller could then get signer.public_key). Changing this likely doesn't make sense anymore, just mentioning.

lukpueh added a commit to lukpueh/in-toto that referenced this issue Jul 4, 2023
@lukpueh
runlib: add signer kwarg to run and record methods (WIP)
a9b87b0 
IMPORTANT: Requires unlreleased securesystemslib code from
secure-systems-lab/securesystemslib#590

Adds optional signer (Signer) arg to runlib's run/record functions, as
alternative way of signing resulting link metadata, instead of using
signing_key, gpg_keyid, or use_default_gpg.

Closes in-toto#532

**Addtional notes:**

In in_toto_record_stop, the public_key (Key) field of the signer is used
to verify the preliminary link metadata file. The field is not yet part
of the interface, although all signer implementations in secureystemslib
have it: secure-systems-lab/securesystemslib#605

The patch aims to be minimally invasive, and thus barely refactors any
of the existing signing argument handling in the relevant functions.
Although, it was tempting to simplify the code, it turned out harder
than thought, and therefor not worth the effort, given that these
arguments are bound to
deprecation.

This is patch is part of a series of patches to prepare for the planned
removal of securesystemslib legacy interfaces, see
secure-systems-lab/securesystemslib#604 for details.

**TODO (follow-up)**
- add deprecation warning for legacy key args and legacy gpg formats
  (only show on API use, not on CLI use!)

- add Metadata.verify(public_key: Key), if necessary (maybe we can just
  do this in runlib/verifylib), and use in in_toto_record_stop instead
  of passing `signer.public_key.to_dict()` to `Metadata.verify_signature(...)`

- prepare for ssslib legacy interface removal in other parts of in_toto:
  - in-toto-run, in-toto-record (in-toto#533)
  - verifylib / in-toto-verify
  - in-toto-sign
  - Metadata API / docs
  - ...

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh mentioned this issue Jul 4, 2023
Merged
@lukpueh
Copy link
Member Author

lukpueh commented Jul 17, 2023

This does mean that from_priv_key_uri() could have been a Signer constructor (and caller could then get signer.public_key). Changing this likely doesn't make sense anymore, just mentioning.

Do you mean, instead of Signer.from_priv_key_uri(uri, public_key, secrets_handler) it could have been Signer(uri, public_key, secrets_handler)?

@lukpueh
Copy link
Member Author

lukpueh commented Sep 28, 2023

On a related note: Any idea how to actually do this? AFAICS, Python only supports defining abstract properties not attributes. This means that Signer subclasses also need to implement public_key as @property, which does not seem so desirable.

Should we maybe just define it via docs?

lukpueh added a commit to lukpueh/in-toto that referenced this issue Oct 25, 2023
@lukpueh
runlib: add signer kwarg to run and record methods
4bf538e 
Adds optional signer (Signer) arg to runlib's run/record functions, as
alternative way of signing resulting link metadata, instead of using
signing_key, gpg_keyid, or use_default_gpg.

Closes in-toto#532

**Addtional notes:**

In in_toto_record_stop, the public_key (Key) field of the signer is used
to verify the preliminary link metadata file. The field is not yet part
of the interface, although all signer implementations in secureystemslib
have it: secure-systems-lab/securesystemslib#605

The patch aims to be minimally invasive, and thus barely refactors any
of the existing signing argument handling in the relevant functions.
Although, it was tempting to simplify the code, it turned out harder
than thought, and therefor not worth the effort, given that these
arguments are bound to
deprecation.

This patch is part of a series of patches to prepare for the planned
removal of securesystemslib legacy interfaces, see
secure-systems-lab/securesystemslib#604 for details.

**TODO (follow-up)**
- add deprecation warning for legacy key args and legacy gpg formats
  (only show on API use, not on CLI use!)

- add Metadata.verify(public_key: Key), if necessary (maybe we can just
  do this in runlib/verifylib), and use in in_toto_record_stop instead
  of passing `signer.public_key.to_dict()` to `Metadata.verify_signature(...)`

- prepare for ssslib legacy interface removal in other parts of in_toto:
  - in-toto-run, in-toto-record (in-toto#533)
  - verifylib / in-toto-verify
  - in-toto-sign
  - Metadata API / docs
  - ...

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh added a commit to lukpueh/in-toto that referenced this issue Oct 25, 2023
@lukpueh
runlib: add signer kwarg to run and record methods
1c23378 
Adds optional signer (Signer) arg to runlib's run/record functions, as
alternative way of signing resulting link metadata, instead of using
signing_key, gpg_keyid, or use_default_gpg.

Closes in-toto#532

**Addtional notes:**

In in_toto_record_stop, the public_key (Key) field of the signer is used
to verify the preliminary link metadata file. The field is not yet part
of the interface, although all signer implementations in secureystemslib
have it: secure-systems-lab/securesystemslib#605

The patch aims to be minimally invasive, and thus barely refactors any
of the existing signing argument handling in the relevant functions.
Although, it was tempting to simplify the code, it turned out harder
than thought, and therefor not worth the effort, given that these
arguments are bound to
deprecation.

This patch is part of a series of patches to prepare for the planned
removal of securesystemslib legacy interfaces, see
secure-systems-lab/securesystemslib#604 for details.

**TODO (follow-up)**
- add deprecation warning for legacy key args and legacy gpg formats
  (only show on API use, not on CLI use!)

- add Metadata.verify(public_key: Key), if necessary (maybe we can just
  do this in runlib/verifylib), and use in in_toto_record_stop instead
  of passing `signer.public_key.to_dict()` to `Metadata.verify_signature(...)`

- prepare for ssslib legacy interface removal in other parts of in_toto:
  - in-toto-run, in-toto-record (in-toto#533)
  - verifylib / in-toto-verify
  - in-toto-sign
  - Metadata API / docs
  - ...

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh mentioned this issue Nov 2, 2023
Open
@jku
Copy link
Collaborator

jku commented Jan 17, 2024 •
edited
Loading

This means that Signer subclasses also need to implement public_key as @property, which does not seem so desirable.

I think the property is probably fine -- we want Signer.public_key to be recognised by type checkers so there has to be something in Signer (so docs only is not great)...

@lukpueh lukpueh added the contribfest Issues for KubeCon EU contribfest label Mar 14, 2024
@jku jku mentioned this issue Mar 22, 2024
Closed
@terricain terricain mentioned this issue Mar 22, 2024
Merged
@lukpueh lukpueh mentioned this issue Jun 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
contribfest Issues for KubeCon EU contribfest good first issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add public key attribute to Signer interface terricain/securesystemslib
2 participants
@jku @lukpueh