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

expose x509 signing capabilities for apptainer's core #149

Closed
wants to merge 1 commit into from

Conversation

fnikolai
Copy link
Contributor

@fnikolai fnikolai commented Oct 4, 2022

This commit builts over #147 to expose signing and verification capabilities to the apptainer's core.

The key idea is that we replace

func (gs *groupSigner) signWithEntity(e *openpgp.Entity) (sif.DescriptorInput, error) 

with

func (gs *groupSigner) signWithEntity(e interface{}) (sif.DescriptorInput, error) 

so that images can be signed either by PGP or X509.

Additionally, it abstracts the signing and verification methods for PGP and X509 behind "dispatcher" functions.

Signed-off-by: Fotis Nikolaidis nikolaidis.fotis@gmail.com

@DrDaveD
Copy link
Contributor

DrDaveD commented Oct 4, 2022

Darn, I thought all the changes to sif for this feature were already included in #147. I thought you had already submitted a PR for the apptainer piece that was ready to go. I guess I shouldn't have made the tag yet. Is this the last piece? I'm trying to keep the version numbers mostly in sync with sylabs/sif.

Does this change the API in a way that's not backward compatible? Would this be able to be used as-is with an older apptainer? I do see some upper-case functions changing, and I'm not sure that's a good idea.

@fnikolai
Copy link
Contributor Author

fnikolai commented Oct 4, 2022

Indeed these changes had to be part of the previous commit, but something went wrong with the squash.

The API is backward compatible -- The definition of exposed APIs is changed to accept an interface and should not cause issues for the caller.

Edit: I 've updated the commit using "WithOptions*" instead of interfaces. Besides backward compatibility, this approach also avoids potential panics caused by erroneous arguments.

== Sign ==
OptSignWithEntity
OptSignWithX509Issuer

== Verify ==
OptVerifyWithKeyRing
OptVerifyWithX509Cert

The respective commit for apptainer is:
fnikolai/apptainer#3

All accounted,I think the best idea, for the time being, is to keep it in a different x509 branch until I commit the changes on the core repository.

For the core repository, I'll use the replace directive in go.mod to point to the x509 branch, and if everything goes as expected, we can merge the x509 branch into the main.

I used a similar technique for the fork.
https://github.com/fnikolai/apptainer/blob/874738ebe76c870becb1d0cef3be794f42df3f4e/go.mod

replace github.com/apptainer/sif/v2 => github.com/fnikolai/sif/v2 v2.0.0-20221005101013-00d06dca77f1

Signed-off-by: Fotis Nikolaidis <nikolaidis.fotis@gmail.com>
@DrDaveD
Copy link
Contributor

DrDaveD commented Oct 5, 2022

So you can use the fnikolai/sif:x509 branch, is that correct? We don't need an apptainer/sif:x509 branch?

@fnikolai
Copy link
Contributor Author

fnikolai commented Oct 5, 2022

That's correct. From the perspective of apptainer/apptainer:x509, it shouldn't matter whether the modified go.mod points to fnikolai/sif:x509 or to apptainer/sif:x509.

However, before the apptainer/apptainer:x509 is merged to apptainer/apptainer:main, the go.mod should be fixed to point back to apptainer/sif:main.

I can see the following strategies:

  1. I make PR for apptainer/apptainer:x509 branch. When you release the v2.8.1 of sif, we change the go.mod and merge it with apptainer/apptainer:main.

  2. Rebuilt the v2.8.0 of sif with this commit, and then I make a PR for apptainer/apptainer:main . According to your CI, this can happen if you do the merge and then re-push the v2.8.0 tag.

  3. Create v2.8.1 of sif with this commit, and then I make a PR for apptainer/apptainer:main (that's ugly because it will break your versioning semantics).

P.S This CI tool seems nice for ensuring backward compatibility on the exposed API.
https://dev.to/vearutop/identifying-breaking-changes-in-pull-request-of-a-go-library-1db2

@DrDaveD
Copy link
Contributor

DrDaveD commented Oct 5, 2022

Changing the contents of a tag may be uglier than breaking versioning semantics. So I don't like strategy 2 very much.

I don't see a point for having an apptainer/apptainer:x509 branch. However I am now thinking that an apptainer/sif:x509 branch may be the way out of this dilemma. We could do do all sif x509 changes there without tags, and change the apptainer/apptainer go.mod to point to that branch with a particular commit for now (or we could use tags that have x509 added to the name). Then once Sylabs gets around to making a 2.9 tag we could merge the x509 branch in with it and switch to back to using tags.

Shall I go ahead and create an apptainer/sif:x509 branch, and you change this PR to be against that branch?

@fnikolai
Copy link
Contributor Author

fnikolai commented Oct 5, 2022

Sounds good. Yes, please go ahead with the branch.

@DrDaveD
Copy link
Contributor

DrDaveD commented Oct 5, 2022

Alright it is created.

@fnikolai
Copy link
Contributor Author

fnikolai commented Oct 5, 2022

Done. I started a new PR #150 instead of rebasing this one (just for the sake of keeping the discussion).

Once merged, I'll update the fnikolai/apptainer:x509 and make a PR for apptainer/apptainer:main with the x509 tag.

This PR can be closed.

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.

2 participants