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

certs: Adding additional functionality. #206

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

larrydewey
Copy link
Contributor

  • Made X509 within Certificate pub (requires major version bump).
  • Added in a bunch of additional support for translating between Certificate, openssl::X509, ca::Chain, and Chain

@larrydewey larrydewey self-assigned this Jul 26, 2024
@auto-assign auto-assign bot requested a review from ryansavino July 26, 2024 14:52
@larrydewey larrydewey added the enhancement New feature or request label Jul 26, 2024
@larrydewey larrydewey force-pushed the enhance-cert branch 2 times, most recently from 184de63 to 8eb3d32 Compare July 26, 2024 15:00
@larrydewey
Copy link
Contributor Author

/cc @fitzthum (just FYI)

@tylerfanelli
Copy link
Member

Not against the change, but what's the purpose of making X509 public?

@larrydewey
Copy link
Contributor Author

Not against the change, but what's the purpose of making X509 public?

It was originally to provide access to the methods belonging to the X509, but I found a better way around that. Let me know what you think about this approach.

@larrydewey larrydewey force-pushed the enhance-cert branch 4 times, most recently from 6ed03b0 to 9182254 Compare July 29, 2024 18:35
Copy link
Member

@tylerfanelli tylerfanelli left a comment

Choose a reason for hiding this comment

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

There's a lot of assumptions we're making about the formatting of the inputs (for instance, an array should be ARK/ASK/VCEK). Documentation for the rustdocs should make this abundantly clear.

Each conversion trait impl should probably outline this.

Copy link
Member

@DGonzalezVillal DGonzalezVillal left a comment

Choose a reason for hiding this comment

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

LGTM!

@DGonzalezVillal
Copy link
Member

There's a lot of assumptions we're making about the formatting of the inputs (for instance, an array should be ARK/ASK/VCEK). Documentation for the rustdocs should make this abundantly clear.

Each conversion trait impl should probably outline this.

ASK/ARK/VEK, but yes I agree, it should be made clear for users

- Added in a bunch of additional support for translating between
  `Certificate`, `openssl::X509`, `ca::Chain`, and `Chain`

- Adding fixes per Tyler's suggestions

Signed-off-by: Larry Dewey <larry.dewey@amd.com>
@larrydewey
Copy link
Contributor Author

Changes have been made on the latest commit.

@larrydewey larrydewey requested a review from tylerfanelli July 29, 2024 18:59
@@ -29,6 +31,77 @@ impl<'a> Verifiable for &'a Chain {
}
}

#[cfg(feature = "openssl")]
impl From<(X509, X509)> for Chain {
Copy link
Member

Choose a reason for hiding this comment

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

I think the ordering of the tuples/arrays/etc should be ARK/ASK rather than ASK/ARK.

Copy link
Contributor Author

@larrydewey larrydewey Jul 30, 2024

Choose a reason for hiding this comment

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

I don't disagree. I would like the root key first, as well. However, we need to follow the standard already presented by the AMD Key Distribution Server (KDS) which explicitly returns (ASK, ARK) in PEM format:

https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/57230.pdf#page=17

@larrydewey larrydewey requested a review from tylerfanelli July 30, 2024 14:06
@tylerfanelli tylerfanelli merged commit 4f8cbc7 into virtee:main Jul 31, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants