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

Feat/rsa keys #59

Merged
merged 17 commits into from
Sep 20, 2024
Merged

Feat/rsa keys #59

merged 17 commits into from
Sep 20, 2024

Conversation

puffitos
Copy link
Collaborator

Motivation

Mostly resolves #57. Some additional E2E testing should be done to lock this feature down.

Changes

New features:

  • The verifyContainer method can now verify containers regardless of whether they have been signed with RSA/ECDSA private keys. A function was introduced to create a verifier based on the public key found.
    • no valid verifier -> error -> error verifying the container -> container doesn't run
  • New function to generate RSA keypair for tests

Housekeeping changes:

  • Refactored tests to use constants & key paths instead of hardcoding the path in the signcontainer function
    • this opens up the path for simpler E2E tests; more on than on the next PR
  • Refactored the framework's signContainer method to use the sign package directly; it's simpler to use.
  • Makefile is now more robust - uses a Port variable and has the additional DNS fix, which was unfortunately necessary for mac machines running the e2e tests (at least for mine 😓 )
  • bumped to go 1.23 & cosign 2.4
  • made the framework's cleanup a bit more resilient and forgiving, to allow for simpler use

Tests done

Two new E2E test cases were added to show that this does indeed work. Some manual testing was done to check how to import keys to cosing, sign and verify containers with those keys.

This allows the test framework to generate RSA keys

Signed-off-by: Bruno Bressi <[email protected]>
The port 5000 is used in mac for some other server.
Additionally bumped dependencies & code to go 1.23
Signed-off-by: Bruno Bressi <[email protected]>
Also moved port back to 5000

Signed-off-by: Bruno Bressi <[email protected]>
To make the tests easier to maintain, a variable was introduced for the port used in the ephemeral private registry used.
This makes the tests somewhat easier to maintain and read.
The cleanup method can now be called always when a test is run using the framework, as it cleans up whatever is there and ignores the rest.
The keys had to be also imported to the cosign format to be usable for signing containers. Additionally, this commit refactors the signing method to use the CLI directly and not the cobra command, which was kind of unintuitive.

An additional test, which doesn't run per default was added to test whether the sign method really works.
Signed-off-by: Bruno Bressi <[email protected]>
Since the switch to the `sign` module, the signatures of the ephemeral images being used in tests were not uploaded to the repository.
This resulted in test failure, as the public key had no signature to verify.

Additionally, the errors with the RSA private key not being suited for image signing and verification are also solved in this commit.
The proper encoding algorithms are now used and the the correct values are returned. The imported public key and the generated one are now the same,
and the signing private key has the correct header now.

WIP.
A simple test locke behind an env variable to test whether an RSA key can be
used to sign a container image. In the future, this test should be an autonomous integration test
and not be connected to the busybox image created during the E2E preparation
Housekeeping commit to refactor the tests so they use the new keypath
argument, which allows them more flexibility and opens up for a future
refactoring to simplify the test suite and allow to run the same test
suite for multiple input keys (ECDSA, RSA).
@puffitos puffitos added enhancement New feature or request dependencies Pull requests that update a dependency file labels Sep 16, 2024
@puffitos puffitos self-assigned this Sep 16, 2024
This was a typo

Signed-off-by: Bruno Bressi <[email protected]>
Copy link
Owner

@eumel8 eumel8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

* refactor: own struct for keys

The tests have been refactored to use a dedicated struct for the private
and public keys, which contains the key itself and the path to it.

This will allow a bigger refactoring of the E2E tests, so that each test case can
be run independently of what type of key is used for signing & validation

Signed-off-by: Bruno Bressi <[email protected]>

* refactor: use private key variable

Instead of hardcoding the path in all tests, the value is derived from the previously
unused private key variable returned. This way, the tests can now be refactored to run
by only passing the key creation function

Signed-off-by: Bruno Bressi <[email protected]>

* refactor: [WIP] framework wraps testing.T

The framework struct has been refactored to abstract the golang testing framework.
This allows the E2E test cases to be written without having to create a new framework for each test.
The framework functions now do not have to do a lot of micromanagement and cleanup; they just check
whether an error has happened and they return. This allows for new functions to be written without having
to think about whether to fail the test or not.

The cleanup function takes care of the final step; cleaning up everything and then deciding whether the test
failed or passed.

Additionally, a new type is introduced, which will be used to wrap the tests cases, so they can be
run used t.Run.

* refactor: use new testing schema

The test cases are now refactored to accept a signing function, so that
the same test can be run regardless of RSA/ECDSA key without having to
write too much duplicate code.

The new fuction type is used for the signing function and each test case
must now return the set of actions required for the use case to be
tested, wrapped in a func which returns testing.T, so it may be run by
the t.Run method.

* chore: added E2E variable

Added variable so that the additional E2E test is also executed. This
test must be refactored in a future commit/ removed, as it depends on an
image already being present on the machine running the test.

* test: added rsa tests cases

Each case tests for ECDSA keys is now also tested for RSA keys.

The tests were also accelerated by reducing the delay between checks
from 5s to 500m

Signed-off-by: Bruno Bressi <[email protected]>

---------

Signed-off-by: Bruno Bressi <[email protected]>
@eumel8 eumel8 merged commit af61ddd into main Sep 20, 2024
12 checks passed
@eumel8 eumel8 deleted the feat/rsa-keys branch September 20, 2024 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only ECDSA keys are accepted - RSA keys should be accepted as well
2 participants