Allow accessing odcs repos via https

[tkdchen]

Signed-off-by: Chenxiong Qi cqi@redhat.com

Maintainers will complete the following section

• Commit messages are descriptive enough
• Code coverage from testing does not decrease and new code is covered
• JSON/YAML configuration changes are updated in the relevant schema
• [n/a] Changes to metadata also update the documentation for the metadata
• Pull request has a link to an osbs-docs PR for user documentation updates
• New feature can be disabled from a configuration file

[lgtm-com]

This pull request introduces 2 alerts when merging 7779b0a into ef43ab2 - view on LGTM.com

new alerts:

• 2 for Unused import

[tkdchen]

Choosing /opt/osbs-build instead of /tmp/ is because bandit reported the hardcoded /tmp/ issue originally.

Does atomic-reactor already have a dedicated directory to hold such files used during build time?

[ben-alkov]

Choosing /opt/osbs-build instead of /tmp/ is because bandit reported the hardcoded /tmp/ issue originally.

Does atomic-reactor already have a dedicated directory to hold such files used during build time?

"I Am Not An Expert", but AFAICT, atomic-reactor uses tempfile from the Python 3 libraries for temp files/dirs.

I imagine that bandit's problem is indeed with hard-coding the string 'tmp' - here's a canonical example of tempfile use, modified from the above link:

with tempfile.TemporaryDirectory() as tmpdirname:
    BUILD_TIME_CA_BUNDLE = f'{tmpdirname}/tls-ca-bundle.pem')
    # etc...

TemporaryDirectory has all of the advantages of mkdtemp() ¹, but is usable by a context manager, so that removal of the temp dir and all of its contents is automatic.

EDIT: On further inspection, it seems like all you really want is something like tmpdirname = tempfile.TemporaryDirectory(). This will give you a nicely randomized directory name. Note that the directory is also created. Because of the way it's being passed around, I wouldn't consider that directory name to be truly secure, but at least it'll silence bandit.

Footnotes

1. "Creates a temporary directory in the most secure manner possible. There are no race conditions in the directory’s creation. The directory is readable, writable, and searchable only by the creating user ID." ↩

[tkdchen]

@ben-alkov Thanks for your explanation. The /tmp/... is used as the destination of instruction ADDduring build, it's a location in the image not a place in the builder. So, I think the with tempfile.TemporaryDirectory() as tmpdirname: should not be helpful for this case. I asked that question is because I thought it would be good and explicit for maintainers to have one place than different plugins use different directories.

I chose the /opt/osbs-build instead of workaround the bandit detection. If there is other similar requirement in the future like the ca-bundle.pem, which are used temporarily during the image build, probably we can simply reuse this directory.

[lkolacek]

@tkdchen looks good. I think it might be worth documenting this change also in osbs-docs. How about an integration test? Are we going to merge it before testing it?

[MartinBasti]

with other plugins we are using /tmp I'd prefer to stay consistent here.

Will be /opt/.. propagated to the final image?

[rcerven]

Choosing /opt/osbs-build instead of /tmp/ is because bandit reported the hardcoded /tmp/ issue originally.
Does atomic-reactor already have a dedicated directory to hold such files used during build time?

"I Am Not An Expert", but AFAICT, atomic-reactor uses tempfile from the Python 3 libraries for temp files/dirs.

I imagine that bandit's problem is indeed with hard-coding the string 'tmp' - here's a canonical example of tempfile use, modified from the above link:

with tempfile.TemporaryDirectory() as tmpdirname:
    BUILD_TIME_CA_BUNDLE = f'{tmpdirname}/tls-ca-bundle.pem')
    # etc...

TemporaryDirectory has all of the advantages of mkdtemp() ¹, but is usable by a context manager, so that removal of the temp dir and all of its contents is automatic.

EDIT: On further inspection, it seems like all you really want is something like tmpdirname = tempfile.TemporaryDirectory(). This will give you a nicely randomized directory name. Note that the directory is also created. Because of the way it's being passed around, I wouldn't consider that directory name to be truly secure, but at least it'll silence bandit.

this is about directory during build, tempfile can be created locally in buildroot, but we need to copy it into build itself

Footnotes

1. "Creates a temporary directory in the most secure manner possible. There are no race conditions in the directory’s creation. The directory is readable, writable, and searchable only by the creating user ID." ↩

[rcerven]

with other plugins we are using /tmp I'd prefer to stay consistent here.

Will be /opt/.. propagated to the final image?

yeah we should just use /tmp in build, the same we are doing when hiding ubi.repo file

[tkdchen]

@MartinBasti @rcerven

In the latest update, the ca bundle is copied to /tmp/, and odcs is not checked. I'll continue updating the unit tests. Please review the update in the plugin. Thanks.

[tkdchen]

The original tests are migrated to the new test test_inject_repos.

[MartinBasti]

Shouldn't be this configurable?

[lkolacek]

Code looks good to me. Should be tested with autotest from CLOUDBLD-4622 though

[MartinBasti]

Where is this default value used? I cannot see it in the code

[tkdchen]

The call to get_builder_ca_bundle

https://github.com/containerbuildsystem/atomic-reactor/pull/1597/files#diff-65b2ed390ee23c54fcfe0ca79c38e0068d64e05f5b554e5a113f5c4a80546fc7R123

[MartinBasti]

I see no fallback there, so it will fail (or return None), but it will NOT use /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem as a default path when it's not configured.

[MartinBasti]

@lkolacek @ssalatsk
I dunno why this is marked as N/A
[n/a] New feature can be disabled from a configuration file

I'd like to have a flag to disable injecting CA bundles to dockerfile

Edit: Resolved

[MartinBasti]

https://github.com/containerbuildsystem/osbs-docs/blob/master/docs/admins.rst#server-side-configuration-for-atomic-reactor

Please add also documentation about new config option ^

[MartinBasti]

Please add issue ID to commit msg

[tkdchen]

@MartinBasti

Please add issue ID to commit msg

Updated.

Please add also documentation about new config option ^

Adding the builder_ca_bundle to reactor config makes me think more. If I understand the tls-ca-bundle certificate file correctly, it could includes multiple certificates to address the SSL authentication by multiple external services. For the issue of this PR, it is the ODCS case of accessing repositories via HTTPS. For other potential cases for OSBS in the future, the added /tmp/tls-ca-bundle could also be used as well.

Therefore, after rethinking, from my point of view, it should make much sense to add a flag (should be the same thing you mentioned yesterday) for ODCS integration specifically. That is, whether to use the ca-bundle for accessing repos via HTTPS is determined by two configure options, one is under the odcs section, e.g. enable_https_for_repo_access, and another one is the already added builder_ca_bundle at the top level of the reactor config. The basic logic is:

if odcs.enable_https_for_repo_access:
    if builder_ca_bundle is set:
        inject the ca-bundle
    else:
        raise configuration error

What do you think?

[tkdchen]

@MartinBasti

Setting a feature flag for odcs is not applicable for now, because current solution is to add the sslcacert to every added repos not only the YUM repos generated by ODCS.

So far, the osbs-docs is updated as well and all comments have been addressed already I think.

[MartinBasti]

I'd go just with one bundle and didn't complicate things ODCS vs yum repo url passed by arg

[rcerven]

I'd go just with one bundle and didn't complicate things ODCS vs yum repo url passed by arg

yes please just keep it simple and do it for all repos