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: handle many cert requests #55

Merged
merged 2 commits into from
Sep 23, 2022
Merged

Conversation

rjzak
Copy link
Member

@rjzak rjzak commented Sep 15, 2022

Signed-off-by: Richard Zak [email protected]

@rjzak rjzak force-pushed the multiple_csrs branch 3 times, most recently from 287405e to ba8c702 Compare September 16, 2022 22:50
@rjzak rjzak changed the title feat: handle many cert requests (WiP) feat: handle many cert requests Sep 16, 2022
@rjzak
Copy link
Member Author

rjzak commented Sep 16, 2022

Maybe this should change the version to 0.2.0, since it's an api-breaking change?

@rjzak rjzak marked this pull request as ready for review September 16, 2022 23:06
@rjzak rjzak requested review from a team and bstrie as code owners September 16, 2022 23:06
@rjzak rjzak force-pushed the multiple_csrs branch 2 times, most recently from 139d2c8 to 5942ce1 Compare September 16, 2022 23:17
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@rjzak
Copy link
Member Author

rjzak commented Sep 19, 2022

attest() how gets a Vec<CertReq> and returns Output, with updated tests, and removed content type check (and removed the two tests pertaining to content type checking).

@rjzak
Copy link
Member Author

rjzak commented Sep 19, 2022

Should this be moved to the top of attest() so that all certs have the same Validity?

let now = SystemTime::now();
let end = now + Duration::from_secs(60 * 60 * 24 * 28);
let validity = Validity {
    not_before: Time::try_from(now).or(Err(ISE))?,
    not_after: Time::try_from(end).or(Err(ISE))?,
};

@rjzak rjzak force-pushed the multiple_csrs branch 3 times, most recently from 8bca698 to 4957743 Compare September 19, 2022 20:37
@rjzak
Copy link
Member Author

rjzak commented Sep 19, 2022

@npmccallum added a unit test to make sure attest() works with multiple CertReq objects, and responds with the same amount of Certficiate objects.

src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@rjzak rjzak force-pushed the multiple_csrs branch 2 times, most recently from 79b80eb to 3e20c75 Compare September 20, 2022 20:16
Co-authored-by: Roman Volosatovs <[email protected]>
Co-authored-by: Nathaniel McCallum <[email protected]>
Signed-off-by: Richard Zak <[email protected]>
src/crypto/certreq.rs Outdated Show resolved Hide resolved
src/crypto/certreq.rs Outdated Show resolved Hide resolved
@rvolosatovs
Copy link
Member

@rjzak see rjzak#1 for suggested approach cleaning this up

@rvolosatovs
Copy link
Member

rvolosatovs commented Sep 22, 2022

Should this be moved to the top of attest() so that all certs have the same Validity?

let now = SystemTime::now();
let end = now + Duration::from_secs(60 * 60 * 24 * 28);
let validity = Validity {
    not_before: Time::try_from(now).or(Err(ISE))?,
    not_after: Time::try_from(end).or(Err(ISE))?,
};

I missed this.
@rjzak could you make validity a parameter to attest_request? Then LGTM

src/main.rs Outdated Show resolved Hide resolved
- Remove redundant clones
- Clean-up error handling and setup logging to debug level as part of it
- Reorganize `use` statements according to Enarx guidelines

Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs rvolosatovs merged commit 6684151 into enarx:main Sep 23, 2022
@rjzak rjzak deleted the multiple_csrs branch September 23, 2022 18:24
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.

3 participants