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

Using cert from nssdb in cert-export #4578

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

fmarco76
Copy link
Member

@fmarco76 fmarco76 commented Oct 3, 2023

Certificate and relative CSR are stored in multiple places. The command pki-server cert-export was reading only from the CS.cfg file but has been modified to read from nssdb and from the folder <instance>/conf/certs/<cert_id>.crt.

System certificates are stored in CS.cfg and nssdb. This is redundant,
all operations should use the same source for the certificate which is
the nssdb.

This modify the following command in order to get the certificate from
nssdb:

  [root@pki /] # pki-server cert-export --cert-file <filename>
@fmarco76 fmarco76 requested a review from edewata October 3, 2023 13:44
Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

Please see my comments. There are minor issues but feel free to update & merge. Thanks!

@@ -2009,6 +2009,8 @@ def get_cert_info(self, nickname, token=None):
cert = {}
cert['object'] = cert_obj

cert['data'] = self.get_cert(nickname=nickname, token=token, output_format='base64')
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably can reuse the return value from get_cert() in line 2001, then convert it to base64 with convert_cert().

Copy link
Member Author

Choose a reason for hiding this comment

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

I have tried to use the cert_pem value but it is a binary string and the convert_cert() has some trouble to convert.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, no worries then, we can deal with it another time. Thanks!

if cert_data:
cert_data = pki.nssdb.convert_cert(cert_data, 'base64', 'pem')
else:
crt_path = os.path.join(instance.conf_dir, 'conf', 'certs', f'{cert_id}.crt')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need the conf part since instance.conf_dir is already pointing to the config folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I think we cannot use f-string because the code needs to be able to run on RHEL 8 which only has Python 3.6.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately I think we cannot use f-string because the code needs to be able to run on RHEL 8 which only has Python 3.6.

f-string are available from 3.6 (https://peps.python.org/pep-0498) and in my test VM they works but I have modified with '+' concatenation to avoid compatibility issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we don't need the conf part since instance.conf_dir is already pointing to the config folder.

Not sure on this. In my tests I get printed the location of the cond_dir and where the csr file is read and I get:

Conf dir is /etc/pki/pki-tomcat
Csr path /etc/pki/pki-tomcat/conf/certs/sslserver.csr

Inside the instance I have not a conf folder so for the moment I am leaving. We can change location for the certs if it is the case or create an instance.cert_dir if it is the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

About f-string, looks like you're right. I was comparing Python 3.6 and 3.7 docs, apparently the f-string is described in different sections:
https://docs.python.org/3.7/tutorial/inputoutput.html
https://docs.python.org/3.6/tutorial/inputoutput.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally a Tomcat instance has a conf folder under the instance folder that contains Tomcat config files (e.g. server.xml): https://tomcat.apache.org/tomcat-9.0-doc/introduction.html

Following the same convention, in a PKI the folder would be /var/lib/pki/<instance name>/conf. However, Fedora requires that config files to be stored under /etc, so the /var/lib/pki/<instance name>/conf is actually a link to /etc/pki/pki-tomcat, but here we don't need to add conf in the path anymore since everything under /etc are config files, so it would be redundant.

Yeah, we can certainly add something like certs_dir to define the location for system certs and CSRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reading the issue you wrote the certificate should go in <instance>/conf/certs folder and I was thinking that <instance> is /etc/pki/<instance_name> but I was wrong, that is the conf folder if IIUC.

I will fix this directly in master. Thanks for the clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

base/server/python/pki/server/cli/cert.py Outdated Show resolved Hide resolved
The command pki-server cert-export will read the certificate and the
relative request from the "<instance>/config/certs" folder if not found
in other places
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@fmarco76
Copy link
Member Author

fmarco76 commented Oct 4, 2023

@edewata Thanks!

@fmarco76 fmarco76 merged commit f54a966 into dogtagpki:master Oct 4, 2023
151 of 152 checks passed
@fmarco76 fmarco76 deleted the RedundantCerts branch October 4, 2023 10:07
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