Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Add ssl support #356

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Add ssl support #356

merged 1 commit into from
Jul 30, 2020

Conversation

mdellweg
Copy link
Member

@mdellweg mdellweg commented Jul 7, 2020

No description provided.

@pulpbot
Copy link
Member

pulpbot commented Jul 7, 2020

Attached issue: https://pulp.plan.io/issues/6845

Attached issue: https://pulp.plan.io/issues/6847

Attached issue: https://pulp.plan.io/issues/6845

@mdellweg
Copy link
Member Author

mdellweg commented Jul 7, 2020

This is based on and should replace: #325

RUN yum install -y epel-release ;\
yum makecache fast ;\
yum update -y ;\
RUN yum install -y epel-release &&\
Copy link
Member Author

Choose a reason for hiding this comment

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

While i strongly believe this change should be there, it seems to consistently break package upgrade tests.
@mikedep333 any thoughts?

Rationale: Without it, molecule produced improper images due to a failed dnf install and even refrained to recreate that layer in subsequent calls.

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 figured that we probably do not want do the Dockerfile preparation stuff with the upgrade tests, as those containers should have gone through this process already.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW: molecule retries to build these images three times. But docker does not rebuild this layer if it thinks it was successful, thereby defeating the retry mechanism.

@mdellweg
Copy link
Member Author

mdellweg commented Jul 7, 2020

This time the ci failure is a fluke.

Copy link
Member

@dkliban dkliban left a comment

Choose a reason for hiding this comment

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

I tested all the new options and found one required change.

The certificate that is being generated always uses ansible_fqdn[0] and not the pulp_webserver_httpd_servername.

I also noticed that all the certificates and keys (root and webserver) are owned by root:root. The main nginx process is started as user root and it starts a child process as user nginx. This allows it to read the certificates. I will investigate if we should be starting nginx differently.

[0] https://github.com/pulp/pulp_installer/pull/356/files#diff-7fae6310ffe71807d431c3e3e78dbbc3R25

@mdellweg mdellweg force-pushed the add_ssl_support branch 2 times, most recently from 907545f to 18b870a Compare July 13, 2020 08:41
Copy link
Member

@dkliban dkliban left a comment

Choose a reason for hiding this comment

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

I tested that the latest changes use the pulp_webserver_httpd_servername when generating the SSL cert. Thank you!

@mdellweg mdellweg force-pushed the add_ssl_support branch 6 times, most recently from 4ecd270 to b99bc2a Compare July 21, 2020 09:38
@mikedep333 mikedep333 self-requested a review July 22, 2020 14:36
Enable HTTPS by default when deploying a new pulp server. One can either
specify the value of the certificate and the key. Or, if none available,
can have the installer generating them.

Support has been added for both nginx and apache.

fixes #6845
https://pulp.plan.io/issues/6845
fixes #6847
https://pulp.plan.io/issues/6847

Co-Authored-By: Matthias Dellweg <mdellweg@redhat.com>
Comment on lines +48 to +50
ssl_protocols TLSv1.2;
ssl_ciphers 'ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256';
ssl_prefer_server_ciphers on;
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to specify these? If we do not, will the system use some (sane/secure) distro default or system-wide crypto policy? If so, that would be much preferable.

Comment on lines +46 to +47
SSLProtocol TLSv1.2
SSLCipherSuite ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to specify these? If we do not, will the system use some (sane/secure) distro default or system-wide crypto policy? If so, that would be much preferable.

Copy link
Member Author

Choose a reason for hiding this comment

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

That part was done by @Spredzy and i have no idea. So any expert on the matter, please weigh in.

Comment on lines +11 to +41

'pulpcore-webserver' do |webserver|
describe port(80) do
it { should be_listening }
end

describe port(443) do
it { should be_listening }
end

describe http('http://localhost/pulp/api/v3/status',
ssl_verify: false) do
its('status') { should eq 301 }
end
end

describe http('http://localhost/pulp/api/v3/status',
ssl_verify: false, max_redirects: 1) do
its('status') { should eq 200 }
its('body') { should match /database_connection/ }
end
end

describe http('https://localhost/pulp/api/v3/status',
ssl_verify: false) do
its('status') { should eq 200 }
its('body') { should match /database_connection/ }
end
end

end
Copy link
Member

@mikedep333 mikedep333 Jul 29, 2020

Choose a reason for hiding this comment

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

Nice! But I thought this was failing due to the container/systemd namespace issue on GHA/Ubuntu. I wrote a long ticket after it blocked other changes I was working on.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, that is the reason why my machine turns into a heir dryer, once molecule containers are installed. Do you think, we can at least ratelimit the restart attempts of pulp_services?

Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

See previous comments, but I still approve.

@dkliban dkliban merged commit 2fa83cd into pulp:master Jul 30, 2020
@mdellweg mdellweg deleted the add_ssl_support branch July 30, 2020 13:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants