Skip to content

Add ca certificates bp#352

Merged
arjun024 merged 4 commits intopaketo-buildpacks:mainfrom
accrazed:add-ca-certificates-bp
Aug 24, 2021
Merged

Add ca certificates bp#352
arjun024 merged 4 commits intopaketo-buildpacks:mainfrom
accrazed:add-ca-certificates-bp

Conversation

@accrazed
Copy link
Copy Markdown
Contributor

this PR resolves #214, adding the CA Certificates buildpack to the python buildpack order grouping.

@accrazed accrazed requested review from a team and thitch97 August 19, 2021 15:07
server_address = ('0.0.0.0', 8080)
httpd = http.server.HTTPServer(server_address, http.server.SimpleHTTPRequestHandler)
httpd.socket = ssl.wrap_socket(httpd.socket, server_side=True, certfile='cert.pem', keyfile='key.pem', ssl_version=ssl.PROTOCOL_TLS)
httpd.serve_forever() No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do not think this app verifies the client against the CA certificate loaded by the buildpack.
Here's what I do

cd AccraZed/python/integration/testdata/ca_cert_apps
pack build caimg -p no_package_manager -b <your-cnb>
docker run --init -it -p 8080:8080 --env PORT=8080 --env SERVICE_BINDING_ROOT="/bindings" -v "$(pwd)/bindings:/bindings/ca-certificates" caimg

Now I expect (1) the following to fail with a certificate error

curl -k https://localhost:8080

and (2) the following to succeed

curl --cert client_certs/cert.pem --key client_certs/key.pem --cacert client_certs/ca.pem https://localhost:8080

But, both of them succeeds. This means the client auth is not checked.

Looking at the python api, One way might be to set cert_reqs=ssl.CERT_REQUIRED and ca_certs to the path to the system trust store.
ssl.get_default_verify_paths() might also be able to provide the system store path. If there's no straight-forward way, I'm also okay if you use SSL_CERT_DIR (see ca-cert README) directly.

The same issue exists for the flask apps. You can take a look at ca-certificate test app in nodejs or go to compare the expected behavior

Copy link
Copy Markdown
Member

@arjun024 arjun024 Aug 20, 2021

Choose a reason for hiding this comment

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

I took a swing at this and looks like SSLContext.load_default_certs() seems to work.

# ssl
context = ssl.SSLContext(ssl.PROTOCOL_TLS)
context.verify_mode = ssl.CERT_REQUIRED
context.load_default_certs(ssl.Purpose.CLIENT_AUTH)
context.load_cert_chain(certfile='cert.pem', keyfile="key.pem")
# wrap http
server_address = ('0.0.0.0', 8080)
httpd = http.server.HTTPServer(server_address, http.server.SimpleHTTPRequestHandler)
httpd.socket = context.wrap_socket(httpd.socket, server_side=True)
httpd.serve_forever()

@accrazed accrazed requested a review from arjun024 August 20, 2021 18:12
httpd = http.server.HTTPServer(server_address, Handler)
httpd.socket = ssl.wrap_socket(
httpd.socket,
ca_certs='/bindings/ca-certificates/ca.pem',
Copy link
Copy Markdown
Member

@arjun024 arjun024 Aug 20, 2021

Choose a reason for hiding this comment

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

You shouldn't be referring to the ca certificate directly that's volume-mounted.
The integration test is making sure that the python buildpack (via its constituent ca-cert cnb) loaded the certificate present at /bindings/ca-certificates/ca.pem to the system truststore. So the test app should make use of the system trust store via python's API for it

Test apps should now require client certificates in order to function correctly
@accrazed accrazed requested a review from arjun024 August 24, 2021 13:27
@arjun024 arjun024 merged commit 13d1618 into paketo-buildpacks:main Aug 24, 2021
@robdimsdale robdimsdale added the semver:minor A change requiring a minor version bump label Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver:minor A change requiring a minor version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CA Certificates Buildpack to Group Ordering

3 participants