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

Copy SSL certs into Windows studio #6844

Merged
merged 2 commits into from Aug 23, 2019

Conversation

@chefsalim
Copy link
Member

commented Aug 13, 2019

This change copies over SSL certs from a hab cert cache from outside the studio when creating a new Windows studio. This allows certs to be seamlessly propagated into the Studio.

Signed-off-by: Salim Alam

Salim Alam
Signed-off-by: Salim Alam <seasalim>
@chefsalim chefsalim requested review from baumanj and eeyun as code owners Aug 13, 2019
@chefsalim chefsalim requested a review from mwrock Aug 13, 2019
@chef-expeditor

This comment has been minimized.

Copy link

commented Aug 13, 2019

Hello chefsalim! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@mwrock
mwrock approved these changes Aug 13, 2019
Copy link
Contributor

left a comment

This will work for a local studio but for docker (on both mac and windows) we will need to make sure to mount the local ssl cache. See

if let Ok(cache_artifact_path) = henv::var(ARTIFACT_PATH_ENVVAR) {

Copy link
Contributor

left a comment

I have a few concerns, but defer to @mwrock on the Windows-specific details.

@@ -71,6 +73,9 @@ pub fn start_docker_studio(_ui: &mut UI, args: &[OsString]) -> Result<()> {
volumes.push(format!("{}:{}/{}",
cache_artifact_path, mnt_prefix, CACHE_ARTIFACT_PATH));
}
if let Ok(cache_ssl_path) = henv::var(SSL_PATH_ENVVAR) {
volumes.push(format!("{}:{}/{}", cache_ssl_path, mnt_prefix, CACHE_SSL_PATH));

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

Instead of "{}/{}", should we use Path::join instead for cross-platform pathing (both here and above)?

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 13, 2019

Author Member

That seems reasonable, however I'm trying to make the minimum change possible in order to minimize the risks of additional refactoring.

This comment has been minimized.

Copy link
@mwrock

mwrock Aug 13, 2019

Contributor

If I am remembering correctly, \ can cause problems in docker mounts.

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 13, 2019

Contributor

If that's the case, it would probably help to document that fact and make a function which takes care of this so that some later well-intentioned person doesn't break things later.

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 13, 2019

Author Member

Cool, I'll add a comment for now

This comment has been minimized.

Copy link
@mwrock

mwrock Aug 14, 2019

Contributor

Either I misremembered or (more likely in this case) docker has fixed this because I just validated \ works fine in volume mounts.

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 23, 2019

Author Member

Still probably safer to leave it as is than risk some regression scenario

Err(_) => {
let path = fs::cache_ssl_path(None::<&str>);
debug!("Setting {}={}", SSL_PATH_ENVVAR, path.display());
env::set_var(SSL_PATH_ENVVAR, &path);

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

Setting the environment variable like this is a bit surprising, and potentially fragile. If we want to make sure there's a value for this environment variable set before we spawn the actual studio process, I think it would be better to set it in inner::start, where we manipulate the rest of the environment we're creating for the exec'd process.

I can see someone easily refactoring the environment code in there for better isolation, etc., and totally removing this SSL path in the process, since it was handled remotely.

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 13, 2019

Author Member

Good point, there's probably room to refactor all this, however I'd prefer to not do refactoring or make flow changes in this PR

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

This is all new code, though.

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 13, 2019

Author Member

It's essentially exactly the same pattern as the code above is my point - I'm happy to open an issue to refactor as a separate PR

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

I took a look at the code above this... yup, same pattern 😱

I think we really do need to refactor all this (separate PR is fine), and put some effort into wrapping up #6314 as well.

@@ -17,6 +17,7 @@ use crate::{config,
use habitat_core::AUTH_TOKEN_ENVVAR;

pub const ARTIFACT_PATH_ENVVAR: &str = "ARTIFACT_PATH";
pub const SSL_PATH_ENVVAR: &str = "SSL_PATH";

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

Should this be prefixed with HAB_?

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 13, 2019

Author Member

Ah, this needs to be "CERT_PATH" to match what the studio is actually expecting

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 13, 2019

Author Member

Fixed

if (Test-Path $sslCacheSourcePath) {
Write-HabInfo "Populating SSL certificate cache at $env:HAB_CACHE_SSL_PATH"
Copy-Item -Path $sslCacheSourcePath\* -Destination $env:HAB_CACHE_SSL_PATH
}

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

Why was the SSL_PATH environment variable needed? It doesn't seem to be referenced in this code.

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 13, 2019

Author Member

ARTIFACT_PATH and CERT_PATH are currently only used in the Linux Studio. We don't have exact parity between Windows and Linux Studios atm.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

If this PR is all about Windows, and Windows doesn't use CERT_PATH, why were the Rust changes above to add CERT_PATH required for this?

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 13, 2019

Author Member

The CERT_PATH changes are not strictly required for Windows (atm) - those look like a missed use case to supplement the Linux PR that was done earlier. If and when we bring Windows into parity with Linux for the env, we will be able to leverage those for Windows also.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

In that case, it might be nice to separate these into different PRs.

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 14, 2019

Author Member

OK

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 14, 2019

Author Member

Well nvm, I think this is going to be needed for the Windows Docker Studio scenario after all - specifically, in the error case in enter.rs (where there is no CERT_PATH env defined) we set the env var, and then in docker.rs we use that var for the volume mount.

@chefsalim chefsalim force-pushed the SA/win-ssl branch from be6756b to 6733e1d Aug 13, 2019
Copy link
Contributor

left a comment

I'd echo @christophermaier's comments. Also, we're adding/changing functionality, but we're not adding any tests. It seems like we should.

Err(_) => {
let path = fs::cache_ssl_path(None::<&str>);
debug!("Setting {}={}", SSL_PATH_ENVVAR, path.display());
env::set_var(SSL_PATH_ENVVAR, &path);

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

This is all new code, though.

@@ -17,6 +17,7 @@ use crate::{config,
use habitat_core::AUTH_TOKEN_ENVVAR;

pub const ARTIFACT_PATH_ENVVAR: &str = "ARTIFACT_PATH";
pub const SSL_PATH_ENVVAR: &str = "CERT_PATH";

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

Might as well call this CERT_PATH_ENVVAR now.

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 13, 2019

Author Member

Ok

if (Test-Path $sslCacheSourcePath) {
Write-HabInfo "Populating SSL certificate cache at $env:HAB_CACHE_SSL_PATH"
Copy-Item -Path $sslCacheSourcePath\* -Destination $env:HAB_CACHE_SSL_PATH
}

This comment has been minimized.

Copy link
@christophermaier

christophermaier Aug 13, 2019

Contributor

If this PR is all about Windows, and Windows doesn't use CERT_PATH, why were the Rust changes above to add CERT_PATH required for this?

@chefsalim chefsalim force-pushed the SA/win-ssl branch from 6733e1d to 461ac2f Aug 13, 2019
Signed-off-by: Salim Alam <salam@chef.io>
@chefsalim chefsalim force-pushed the SA/win-ssl branch from 461ac2f to 6784b38 Aug 13, 2019
@chefsalim chefsalim requested a review from christophermaier Aug 14, 2019
@chefsalim chefsalim dismissed christophermaier’s stale review Aug 23, 2019

Refactoring should be separate PR in order to keep the scope of changes to this code small in this PR and not risk regressions more than necessary

@chefsalim chefsalim merged commit a15a85f into master Aug 23, 2019
5 checks passed
5 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #3062 passed (28 minutes, 52 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #190 passed and blocked (46 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@chefsalim chefsalim deleted the SA/win-ssl branch Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.