Skip to content

object: introduce signature abstraction #705

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

Closed
wants to merge 2 commits into from

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Mar 7, 2023

This is work in progress to further facilitate #400. As this introduces (non-breaking) but important contract changes, I would like to have eyes on this early on.

@hiddeco hiddeco force-pushed the signature-abstraction branch 7 times, most recently from 73ebe5c to 95b3408 Compare March 9, 2023 22:28
hiddeco added 2 commits March 17, 2023 21:20
This refactors the existing PGP verification and signing code into an
abstraction which allows multiple signature types to exist in a
backwards compatible manner.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
@hiddeco hiddeco force-pushed the signature-abstraction branch from 11ec329 to e1b64bc Compare March 17, 2023 20:21
@pjbgf pjbgf added this to the v5.8.0 milestone May 11, 2023
@hiddeco
Copy link
Member Author

hiddeco commented May 19, 2023

Talked with @pjbgf about this last week, and due to the lack of time I have to finalize the "nice" user part of SSH signatures on user systems (i.e. by taking the allowed_signers into account).

We thought it may be an option to introduce the abstraction without providing an implementation with a "keyring" like functionality. As the abstraction itself seems sound, and works as is.

Any objections against this @mcuadros? If not, I will rebase the PR and spend one more time thinking about the details of the abstraction itself (e.g. the EntityType versus just type checking the implementation of the interface). Plus add the required tests, of course.

@pjbgf pjbgf modified the milestones: v5.8.0, v5.10.0 Sep 18, 2023
@pjbgf pjbgf modified the milestones: v5.10.0, v5.11.0 Oct 28, 2023
wlynch added a commit to wlynch/go-git that referenced this pull request Feb 13, 2024
crypto.Signer was incorrectly used before. Signer documentation says
that Signer.Sign should be used on digests, whereas we were using this
on message bodies.

To fix this, create our own Signer interface (+ signableObject borrowed
from go-git#705) that describes more accurately what we want.
As before, the expectation is that signer implementations only need to
worry about acting on encoded message bodies rather than needing to
encode objects themselves.

This is technically a breaking change from the previous Signer
implementation, but since this is new and hasn't made it into cut
release yet, this seems like an acceptible change.

Also adds example test showing how signers can be made (uses base64 for
consistent outputs).
wlynch added a commit to wlynch/go-git that referenced this pull request Feb 13, 2024
crypto.Signer was incorrectly used before. Signer documentation says
that Signer.Sign should be used on digests, whereas we were using this
on message bodies.

To fix this, create our own Signer interface (+ signableObject borrowed
from go-git#705) that describes more accurately what we want.
As before, the expectation is that signer implementations only need to
worry about acting on encoded message bodies rather than needing to
encode objects themselves.

This is technically a breaking change from the previous Signer
implementation, but since this is new and hasn't made it into cut
release yet, this seems like an acceptible change.

Also adds example test showing how signers can be made (uses base64 for
consistent outputs).
wlynch added a commit to wlynch/go-git that referenced this pull request Feb 13, 2024
crypto.Signer was incorrectly used before. Signer documentation says
that Signer.Sign should be used on digests, whereas we were using this
on message bodies.

To fix this, create our own Signer interface (+ signableObject borrowed
from go-git#705) that describes more accurately what we want.
As before, the expectation is that signer implementations only need to
worry about acting on encoded message bodies rather than needing to
encode objects themselves.

This is technically a breaking change from the previous Signer
implementation, but since this is new and hasn't made it into cut
release yet, this seems like an acceptible change.

Also adds example test showing how signers can be made (uses base64 for
consistent outputs).
wlynch added a commit to wlynch/go-git that referenced this pull request Feb 13, 2024
crypto.Signer was incorrectly used before. Signer documentation says
that Signer.Sign should be used on digests, whereas we were using this
on message bodies.

To fix this, create our own Signer interface (+ signableObject borrowed
from go-git#705) that describes more accurately what we want.
As before, the expectation is that signer implementations only need to
worry about acting on encoded message bodies rather than needing to
encode objects themselves.

This is technically a breaking change from the previous Signer
implementation, but since this is new and hasn't made it into cut
release yet, this seems like an acceptible change.

Also adds example test showing how signers can be made (uses base64 for
consistent outputs).
@pjbgf pjbgf modified the milestones: v5.11.0, v5.13.0 Aug 1, 2024
traidare pushed a commit to traidare/go-git that referenced this pull request Oct 26, 2024
crypto.Signer was incorrectly used before. Signer documentation says
that Signer.Sign should be used on digests, whereas we were using this
on message bodies.

To fix this, create our own Signer interface (+ signableObject borrowed
from go-git#705) that describes more accurately what we want.
As before, the expectation is that signer implementations only need to
worry about acting on encoded message bodies rather than needing to
encode objects themselves.

This is technically a breaking change from the previous Signer
implementation, but since this is new and hasn't made it into cut
release yet, this seems like an acceptible change.

Also adds example test showing how signers can be made (uses base64 for
consistent outputs).
@github-actions github-actions bot added the stale Issues/PRs that are marked for closure due to inactivity label Feb 14, 2025
@github-actions github-actions bot closed this Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues/PRs that are marked for closure due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants