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

Jump to bottom

Use new ssl certificate kickstart options #1745

Merged
merged 1 commit into from Jan 30, 2019
Merged

Use new ssl certificate kickstart options #1745

merged 1 commit into from Jan 30, 2019

Conversation

larskarlitski
Copy link
Contributor

The repo and url commands can now contain --sslcacert,
--sslclientcert, and --sslclientkey options. Use those when accessing
repositories.

The corresponding change in pykickstart: pykickstart/pykickstart#250

poncovka
poncovka requested changes Jan 2, 2019
View reviewed changes
Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise.

pyanaconda/kickstart.py Outdated
@@ -2506,13 +2506,13 @@ def _getArgsAsStr(self):
return retval

def _getParser(self):
op = KSOptionParser(prog="installclass", version=F27, description="""
op = KSOptionParser(prog="installclass", version=F30, description="""
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, don't change this version.

pyanaconda/kickstart.py Outdated
Require the specified install class to be used for
the installation. Otherwise, the best available
install class will be used.""")

op.add_argument("--name", dest="name", required=True, type=str,
version=F27, help="""
version=F30, help="""
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

pyanaconda/payload/__init__.py
@@ -657,7 +657,11 @@ def _refreshInstallTree(self, url):
else:
proxy_url = None

sslverify = not flags.noverifyssl
sslverify = getattr(self.data.method, "sslcacert", not flags.noverifyssl)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, don't change the type of sslverify. It should be a boolean.

@poncovka
Copy link
Contributor

poncovka commented Jan 2, 2019

Jenkins, test this please.

@poncovka poncovka added master Please, use the `f39` label instead. manual testing required This issue can't be merged without manual testing labels Jan 2, 2019
@larskarlitski
Copy link
Contributor Author

Update fixing the issues @poncovka raised and rebase to latest master. Thanks, and sorry for the obvious mistakes.

poncovka
poncovka reviewed Jan 3, 2019
View reviewed changes
pyanaconda/payload/__init__.py Outdated
@@ -657,7 +657,11 @@ def _refreshInstallTree(self, url):
else:
proxy_url = None

sslverify = not flags.noverifyssl
sslverify = hasattr(self.data.method, "sslcacert") or not flags.noverifyssl
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if this is correct. Shouldn't sslverify be set based on the value of self.data.method.sslcacert? Now it is set based on the existence of the attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value is a url to the cert. If one is given, this code assumes that ssl should be verified (overriding flags.noverifyssl).

Copy link
Contributor

Choose a reason for hiding this comment

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

If self.data.method.sslcacert is None, then hasattr(self.data.method, "sslcacert") returns True. https://docs.python.org/3/library/functions.html#hasattr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, I've been doing too much JavaScript. Sorry for the noise. Made it more explicit now.

@larskarlitski larskarlitski force-pushed the clientcert branch 2 times, most recently from 80e9d83 to e4351c3 Compare January 3, 2019 17:18
@larskarlitski
Copy link
Contributor Author

Rebase to resolve merge conflict.

poncovka
poncovka approved these changes Jan 4, 2019
View reviewed changes
Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Looks good to me now. The unit tests and pylint checks pass.

@poncovka
Copy link
Contributor

poncovka commented Jan 4, 2019

I can confirm that Anaconda doesn't crash, when the options are not set, but I am not sure how to easily test the functionality. Have you tested this pull request, @larskarlitski?

@larskarlitski
Copy link
Contributor Author

larskarlitski commented Jan 6, 2019 •
edited

@poncovka: I'm sorry, I had to revert to the old sslverify line, because it actually needs to contain the path to a certificate. The verify parameter for requests.get is documented as such.

I've added a comment to (hopefully) make this more clear.

I can confirm that Anaconda doesn't crash, when the options are not set, but I am not sure how to easily test the functionality. Have you tested this pull request, @larskarlitski?

To confirm that this works, you need a valid RHEL subscription. It will fill /etc/yum.repos.d/redhat.repo with repositories that have sslcacert, sslclientcert, and sslclientkey set to certificate files deep in /etc/. If you copy those into a url or repo line in a kickstart, anaconda should be able to fetch content from those repos (which are on Red Hat's CDN), which is not possible without the certs.

jkonecny12
jkonecny12 reviewed Jan 9, 2019
View reviewed changes
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me from the code point of view.

poncovka
poncovka reviewed Jan 29, 2019
View reviewed changes
anaconda.spec.in Outdated
@@ -38,7 +38,7 @@ Source0: %{name}-%{version}.tar.bz2
%define libxklavierver 5.4
%define mehver 0.23-1
%define nmver 1.0
%define pykickstartver 3.16-1
%define pykickstartver 3.20-1
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please change it to 3.19?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Also rebased to latest master.

@larskarlitski
Use new ssl certificate kickstart options
a2a5678 
The `repo` and `url` commands can now contain --sslcacert,
--sslclientcert, and --sslclientkey options. Use those when accessing
repositories.
@poncovka
Copy link
Contributor

Jenkins, test this please.

@poncovka poncovka removed the manual testing required This issue can't be merged without manual testing label Jan 30, 2019
@poncovka poncovka merged commit ea307ef into rhinstaller:master Jan 30, 2019
@rvykydal rvykydal mentioned this pull request Mar 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkonecny12 jkonecny12 jkonecny12 left review comments

@poncovka poncovka poncovka approved these changes

Assignees
No one assigned
Labels
master Please, use the `f39` label instead.
Milestone
No milestone
3 participants
@larskarlitski @poncovka @jkonecny12