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.

Sign up for GitHub

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

Jump to bottom

Set X509 SAN with local DNSname/IP/IPv6 #744

Merged
merged 1 commit into from
Feb 29, 2024
Merged

Set X509 SAN with local DNSname/IP/IPv6 #744

merged 1 commit into from
Feb 29, 2024

Conversation

sarroutbi
Copy link
Contributor

@sarroutbi sarroutbi commented Feb 15, 2024 •
edited
Loading

Set X509 Subject Alternative Name with local DNS names (localhost, localhost.domain) and local IPs (127.0.0.1, ::1). It will also include the contact IP stored in configuration

Resolves: #327

@sarroutbi sarroutbi mentioned this pull request Feb 15, 2024
Closed
@sarroutbi sarroutbi force-pushed the 202402151915-set-subject-alternate-name branch 10 times, most recently from 8293088 to 6fd08a6 Compare February 15, 2024 21:58
@sarroutbi
Copy link
Contributor Author

I have verified, by checking on certificates generated, that information is there. In case no additional IP is provided, certificate has next information:

...
            X509v3 Subject Alternative Name: 
                DNS:localhost, DNS:localhost.domain, IP Address:127.0.0.1, IP Address:0:0:0:0:0:0:0:1
...

Meanwhile, if I provide a pair of IPs, they are included appropriately:

...
           X509v3 Subject Alternative Name: 
                DNS:localhost, DNS:localhost.domain, IP Address:127.0.0.1, IP Address:0:0:0:0:0:0:0:1, IP Address:1.2.3.4, IP Address:1.2.3.5
...

@sarroutbi sarroutbi force-pushed the 202402151915-set-subject-alternate-name branch 2 times, most recently from 6201e99 to 6893f77 Compare February 15, 2024 23:05
@sarroutbi sarroutbi changed the title WIP: Set X509 SAN with local DNSname/ip Set X509 SAN with local DNSname/ip Feb 15, 2024
@sarroutbi sarroutbi marked this pull request as ready for review February 15, 2024 23:06
@sarroutbi
Copy link
Contributor Author

/packit retest-failed

6 similar comments
@sarroutbi
Copy link
Contributor Author

/packit retest-failed

@sarroutbi
Copy link
Contributor Author

/packit retest-failed

@sarroutbi
Copy link
Contributor Author

/packit retest-failed

@sarroutbi
Copy link
Contributor Author

/packit retest-failed

@sarroutbi
Copy link
Contributor Author

/packit retest-failed

@sarroutbi
Copy link
Contributor Author

/packit retest-failed

@sarroutbi
Copy link
Contributor Author

/packit retest-failed

@sarroutbi sarroutbi force-pushed the 202402151915-set-subject-alternate-name branch from 6893f77 to 7f8238d Compare February 22, 2024 15:53
@sarroutbi sarroutbi changed the title Set X509 SAN with local DNSname/ip Set X509 SAN with local DNSname/IP/IPv6 Feb 22, 2024
@codecov Codecov
Copy link

codecov bot commented Feb 22, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 51.28205% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 61.60%. Comparing base (ad57804) to head (34813a8).

Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 58.20% <63.33%> (+0.10%) ⬆️
upstream-unit-tests 50.63% <42.85%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
keylime-agent/src/registrar_agent.rs 98.11% <ø> (ø)
keylime-agent/src/main.rs 25.72% <0.00%> (-0.30%) ⬇️
keylime/src/crypto.rs 64.77% <66.66%> (+0.17%) ⬆️

... and 1 file with indirect coverage changes

@ansasaki
Copy link
Contributor

@sarroutbi Sorry, I merged the other PR where I moved the crypto.rs to the keylime/src directory. Could you please rebase and make the changes in the new location?

@sarroutbi
Copy link
Contributor Author

sarroutbi commented Feb 23, 2024 •
edited
Loading

@sarroutbi Sorry, I merged the other PR where I moved the crypto.rs to the keylime/src directory. Could you please rebase and make the changes in the new location?

Hello @ansasaki. No problem at all. This should be easily fixable. Thanks for noticing it

@sarroutbi sarroutbi force-pushed the 202402151915-set-subject-alternate-name branch 4 times, most recently from 650e1b0 to 34813a8 Compare February 23, 2024 18:46
ueno
ueno reviewed Feb 24, 2024
View reviewed changes
keylime-agent/src/main.rs
crypto::generate_x509(
&nk_priv,
&agent_uuid,
Some(contact_ips),
Copy link
Contributor

@ueno ueno Feb 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Now that crypto::generate_x509 is part of the public API, it might make sense to consider switching to the builder pattern so we can maintain compatibility more easily in the future:

crypto::CertBuilder::new(&nk_priv, &agent_uuid)
  .san(&config.agent.contact_ip)
  .build()?

Copy link
Contributor Author

@sarroutbi sarroutbi Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hello @ueno . Thanks for suggestion. I'm a little bit lost regarding this ... where is this "CertBuilder" defined? I don't see it anywhere in the code ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @ueno, I understand your proposal and I think it is a good idea. I'll create an issue for it and merge this PR as it is, then propose a PR to make the certificate generation to follow the builder pattern (before the next release).

Copy link
Contributor

Choose a reason for hiding this comment

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

Created #755 to track this change

@sarroutbi
Set X509 SAN with local DNSname/IP/IPv6
60572f5 
Set X509 Subject Alternative Name with local
DNS names (localhost, localhost.domain) and
local IPs (127.0.0.1, ::1). It will also include
the contact IP stored in configuration

Resolves: keylime#327

Signed-off-by: Sergio Arroutbi <[email protected]>
@sarroutbi sarroutbi force-pushed the 202402151915-set-subject-alternate-name branch from 34813a8 to 60572f5 Compare February 27, 2024 14:29
@sarroutbi sarroutbi requested a review from ueno February 27, 2024 14:52
@ansasaki ansasaki mentioned this pull request Feb 29, 2024
Closed
ansasaki
ansasaki approved these changes Feb 29, 2024
View reviewed changes
Copy link
Contributor

@ansasaki ansasaki left a comment

Choose a reason for hiding this comment

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

Let's merge this PR as it is. The follow-up changes request are tracked in #755

@ansasaki ansasaki merged commit 16bd20b into keylime:master Feb 29, 2024
10 checks passed
@sarroutbi sarroutbi deleted the 202402151915-set-subject-alternate-name branch February 29, 2024 14:24
@kkaarreell kkaarreell mentioned this pull request Mar 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ansasaki ansasaki ansasaki approved these changes

@THS-on THS-on Awaiting requested review from THS-on

@sergio-correia sergio-correia Awaiting requested review from sergio-correia

@lukehinds lukehinds Awaiting requested review from lukehinds

@ueno ueno Awaiting requested review from ueno

Assignees

@ueno ueno

@lukehinds lukehinds

@THS-on THS-on

@sergio-correia sergio-correia

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add localhost, server and contact ip to agent certificate
7 participants
@sarroutbi @ansasaki @ueno @lukehinds @THS-on @sergio-correia @keylime-bot