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_CERT_FILE to studio internal and persist env var #6909

Merged
merged 1 commit into from Sep 6, 2019

Conversation

@smacfarlane
Copy link
Contributor

commented Aug 27, 2019

Closes #6900

This will copy an ssl certificate specified by the SSL_CERT_FILE env variable to the inside of the studio, and set the variable on the inside of the studio to the internal cache.

@chefsalim I think we'll want to pull the certificate copy back into studio::enter in the Rust code. It will cut down on the duplication between shell/powershell and I think it will need to live there anyways in order for the Docker studio to have the same functionality.

I'm going to work on the powershell implementation and add some tests in the mean time.

Signed-off-by: Scott Macfarlane smacfarlane@chef.io

@smacfarlane smacfarlane requested a review from chefsalim Aug 27, 2019
@chef-expeditor

This comment has been minimized.

Copy link

commented Aug 27, 2019

Hello smacfarlane! 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!

@chefsalim

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

@smacfarlane doing the copy in studio enter should be ok as well, but may be less straightforward.

@smacfarlane

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

@smacfarlane doing the copy in studio enter should be ok as well, but may be less straightforward.

I agree, however the Docker studio never hits this code so afaict would need to be solved in studio::enter or studio::docker anyways, but I'm happy to make that a separate PR.

@chefsalim

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@smacfarlane Feel free to do the Docker work in either this PR or separately

@chefsalim

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

LGTM so far..

@@ -259,15 +259,15 @@ function New-Studio {
mkdir $env:HAB_CACHE_SSL_PATH | Out-Null
}

$sslCacheSourcePath = Join-Path $env:SystemDrive "hab\cache\ssl"
if (Test-Path $sslCacheSourcePath) {
if (Test-Path $env:CERT_PATH) {

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Aug 29, 2019

Author Contributor

This change aligns the cert behavior with Linux. CERT_PATH will be set via studio::enter to either C:\hab\cache\ssl or $HOME\.hab\cache\ssl. It can optionally be overridden by $env:CERT_PATH

@smacfarlane smacfarlane marked this pull request as ready for review Aug 29, 2019
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/studio/bin/hab-studio.ps1 Outdated Show resolved Hide resolved
components/studio/bin/hab-studio.ps1 Outdated Show resolved Hide resolved
Copy link
Contributor

left a comment

I'm not done reviewing, but I thought it might be helpful to get some initial feedback.

I noticed a number of warnings in test code. My go-to command when developing is cargo check --all --tests. That should be the fastest way to see if there are any errors or warnings, including test code.

components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/docker.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
Copy link
Contributor

left a comment

Looks great overall. I want to make sure we do a bit more on the test side and address the misleadingly named NO_ARTIFACT_PATH before this merges.

components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
@@ -114,6 +116,18 @@ pub fn start_docker_studio(_ui: &mut UI, args: &[OsString]) -> Result<()> {
args.remove(index);
}

// When a user sets SSL_CERT_FILE, we need to modify the absolute
// path to the file to reflect the location of the file inside the studio
if let Ok(ssl_cert_file) = env::var(SSL_CERT_FILE_ENVVAR) {

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 30, 2019

Contributor

Encapsulating this block in a function would allow us to get it under test. Additionally, you may find the locked_env_var macro useful for testing code that accesses the environment.

components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
@@ -882,6 +881,12 @@ chroot_env() {
env="$env no_proxy=$(echo "$no_proxy" | $bb sed 's/, /,/g')"
fi

if [ -n "${SSL_CERT_FILE:-}" ] \
&& [ -z "${NO_MOUNT}" ] \
&& [ -z "${NO_ARTIFACT_PATH}" ]; then

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 30, 2019

Contributor

I think the name NO_ARTIFACT_PATH is misleading if it affect certs as well. We should update this or else it could cause future bugs and confusion.

This comment has been minimized.

Copy link
@chefsalim

chefsalim Aug 31, 2019

Member

Hmm, the NO_ARTIFACT_PATH test seems unnecessary. I'd be concerned about changing an existing var that might break backward compat. Maybe just remove the NO_ARTIFACT_PATH line here?

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 31, 2019

Contributor

I can't seem to get -N or setting the NO_ARTIFACT_PATH environment variable to actually do what it purports to do:

➤ hab studio --help
…
COMMON FLAGS:
…
    -N  Do not mount the source artifact cache path into the Studio (default: mount the path)
ENVIRONMENT VARIABLES:
…
    NO_ARTIFACT_PATH       If set, do not mount the source artifact cache path (`-N' flag overrides)
➤ hab studio rm
…
➤ env DEBUG=1 hab studio -N enter
…
[1][default:/src:0]# ls /hab/cache/artifacts/
core-acl-2.2.53-20190115012136-x86_64-linux.hart             core-gzip-1.9-20190115013612-x86_64-linux.hart               core-openssl-1.0.2r-20190305210149-x86_64-linux.hart
…

even though when I grep the debug output, I see:

+ NO_ARTIFACT_PATH=true

So, we should probably figure out whether that's a bug and if it has impact here. If it's only supposed to apply to /hab/cache/artifacts and will leave /hab/cache/ssl alone, I agree that we should just omit that check here, but I got the sense that it may have been intended to control the mounting of the entire /hab/cache directory and that the name was from when we didn't have other things cached, but I'm really not sure.

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Sep 3, 2019

Author Contributor

This looks like a copy/paste gone wrong, it should be NO_CERT_PATH, which I do think we want in this series of checks.

@baumanj I wasn't able to replicate the behavior you were seeing of NO_ARTIFACT_PATH, would you mind opening an issue on that?

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 3, 2019

Contributor

What do you see when you pass -N or set NO_ARTIFACT_PATH, @smacfarlane ?

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Sep 3, 2019

Author Contributor

@baumanj If I enter an existing studio, I see an empty directory. If I destroy and create as you did, then I see the TDEPS of the studio, launcher and supervisor which is what I would expect. NO_ARTIFACT_CACHE only controls the mounting of the cache from the host. hab will still use /hab/cache/artifacts to cache everything it downloads, but the artifacts aren't shared with the host so will need to be fetched from builder and won't saved after the studio is removed. Sorry I wasn't more clear this morning.

@smacfarlane smacfarlane requested a review from mwrock Sep 3, 2019
@mwrock
mwrock approved these changes Sep 3, 2019
Copy link
Contributor

left a comment

The powershell looks good to me. Does hab spit out the warn! with the default logging level?

@smacfarlane

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

The powershell looks good to me. Does hab spit out the warn! with the default logging level?

@mwrock It does not, but I'd like to address that in #6799

@smacfarlane smacfarlane requested a review from baumanj Sep 4, 2019

cache_ssl_cert_file(&ssl_cert_dir.to_str().unwrap(), &cert_cache_dir);

assert!(!cert_cache_dir.join(needs_a_name).exists());

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 4, 2019

Contributor

The thing that inspired me to suggest this test was the cert_path.is_file() check in cache_ssl_cert_file. However, as written, this test doesn't exercise that code path. I tried deleting the check and the test still passes. This led me to investigate a bit more, and (after adding env_logger::try_init().ok(); to the top of the test to enable log output), I noticed:

[2019-09-04T17:06:53Z DEBUG hab::command::studio::enter] Caching SSL_CERT_FILE /tmp/.tmpC1VWgY
[2019-09-04T17:06:53Z WARN  hab::command::studio::enter] Unable to cache SSL_CERT_FILE: the source path is not an existing regular file

And reading up on std::fs::copy, led me to find:

This function will return an error in the following situations, but is not limited to just these cases:

  • The from path is not a file.

So, as fart as correctness goes, I think we can remove the cert_path.is_file check, but leave this test to guard against regressions. Alternatively, it might be good to keep the check (and possibly add one that cert_cache_dir is a directory) and use those to provide more informative error messages to users, but that's a separate issue from the automated testing, which should validate correctness (not UX quality) and protect against regressions.

To check that the test is correct, I tried changing

    if let Err(err) = stdfs::copy(cert_file, &cache_file) {

to something that would copy directories (this required the fs_extra crate):

    let mut copy_options = fs_extra::dir::CopyOptions::new();
    copy_options.copy_inside = true;
    copy_options.depth = 100;
    if let Err(err) = fs_extra::dir::copy(cert_file, &cache_file, &copy_options) {

And this still passed because I think the test is not quite checking the thing we want. I think the check we want here is:

Suggested change
assert!(!cert_cache_dir.join(needs_a_name).exists());
assert!(!cert_cache_dir.join(ssl_cert_dir.file_name().unwrap())
.exists());

This fails (as expected) with the fs_extra recursive copy code, but passes with the normal std::fs::copy version (and the is_file check removed).

Also, this allows us to remove needs_a_name entirely.


Ok(())
}
}

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 4, 2019

Contributor

I think there's one other test that we need here. From the std::fs::copy docs:

Note that if from and to both point to the same file, then the file will likely get truncated by this operation.

It would be bad news if we were to accidentally clobber the file the user provided. We shouldn't get into this situation based on how the calling code is now, but the tests for this function shouldn't make assumptions about how it will be called because that could change in the future.

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Sep 5, 2019

Author Contributor

I've added a guard in the cache_ssl_cert_file function, but I'm not sure how we could test that. Since fs::copy isn't guaranteed to truncate the file, inspecting the contents would likely intermittently fail. There aren't any other side effects that I can think of to inspect, since the function is essentially a no-op in this case.

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 5, 2019

Contributor

A test that intermittently fails in the face of incorrect logic is preferable to no test at all. Even better would be making cache_ssl_cert_file return a Result so we can test that we positively catch the conditions where the copy would refer to the same file (whether identical paths, symlinks or hard links).

}

#[test]
fn cache_ssl_cert_file_invalid_cert_cache_dir() -> std::io::Result<()> {

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 4, 2019

Contributor

Sorry, I'm not sure what I was thinking with suggesting this particular test. Since cache_ssl_cert_file doesn't report any errors, there's nothing observable that could happen with passing an invalid cert_cache_dir. We can probably remove this test altogether.

.to_str()
.unwrap()
.to_string();
assert_eq!(std::env::var(SSL_CERT_FILE_ENVVAR), Ok(internal_cert_path));

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 4, 2019

Contributor

This test would be more valuable if mnt_prefix weren't an empty string. As is it is now, we can break update_ssl_cert_file_envvar by changing

env::set_var(SSL_CERT_FILE_ENVVAR,
             Path::new(mnt_prefix).join(CACHE_SSL_PATH)
                                  .join(cert_file_name));

to

env::set_var(SSL_CERT_FILE_ENVVAR,
             Path::new(CACHE_SSL_PATH).join(cert_file_name));

and the test still passes

@smacfarlane

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@baumanj I think I've addressed all your feedback, with the exception of the same-file test. Any advice on how to proceed on that one would be appreciated.

let cert_filename = cert_path.file_name().expect("SSL_CERT_FILE is not a file");
let cache_file = cert_cache_dir.join(&cert_filename);

if cache_file != cert_path {

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 5, 2019

Contributor

This != is checking if the paths are equal, but different paths can refer to the same file via symlinks or hard links. I think that's the most likely way in which we'd end up in this case where the inputs to copy refer to the same file, so I believe it's important to handle it.

Fortunately, there's same-file crate, which appears to do the right thing (and is cross platform), so I think we can use that and be fairly safe.

Note that you'll need to first check whether cert_file exists since same_file::is_same_file will give an error unless both inputs exist and are accessible.


Ok(())
}
}

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 5, 2019

Contributor

A test that intermittently fails in the face of incorrect logic is preferable to no test at all. Even better would be making cache_ssl_cert_file return a Result so we can test that we positively catch the conditions where the copy would refer to the same file (whether identical paths, symlinks or hard links).

@@ -43,6 +45,22 @@ fn set_env_var_from_config(env_var: &str, config_val: Option<String>, sensitive:
}
}

fn cache_ssl_cert_file(cert_file: &str, cert_cache_dir: &Path) {
let cert_path = Path::new(&cert_file);

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 5, 2019

Contributor

I've found myself getting confused when reading the logic of this function, and I think a few naming changes may make it easier to follow:

  1. cert_filefile_to_cache or src_cert, etc.
  2. cert_cache_dirdest_dir
  3. When converting cert_file of type &strPath, re-bind the same name rather than introduce cert_path* (this is allowed and idiomatic) or make the function take a Path as input rather than &str.

These are just suggestions; not required changes

* Since rust has such strong typing, in general I find it useful to put only the purpose of the data in the variable name and leave out words that describe the type.

Copy link
Contributor

left a comment

The only thing that needs to change is the missing dependency in cargo.toml; everything else here is optional

match cache_ssl_cert_file(&ssl_cert_file, &ssl_path) {
Ok(_) => {}
Err(err) => warn!("Unable to cache SSL_CERT_FILE: {}", err),
}

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 5, 2019

Contributor
        if let Err(err) = cache_ssl_cert_file(&ssl_cert_file, &ssl_path) {
            warn!("Unable to cache SSL_CERT_FILE: {}", err);
        }

is a tad simpler/more idiomatic; but the match approach works too

BLDR_URL_ENVVAR,
CTL_SECRET_ENVVAR,
ORIGIN_ENVVAR};

use habitat_core::AUTH_TOKEN_ENVVAR;
use same_file::is_same_file;

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 5, 2019

Contributor

I'm getting

error[E0432]: unresolved import same_file

when I try this. Is there a change to the cargo.toml that didn't get pushed up, by any chance?

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Sep 6, 2019

Author Contributor

🤦‍♂ Fixed!

.to_string()));
}

debug!("Caching SSL_CERT_FILE {}", cert_file);

This comment has been minimized.

Copy link
@baumanj

baumanj Sep 5, 2019

Contributor

In case of errors, this would probably be more useful at the top of the function and also inclusive of the destination. Something like

Suggested change
debug!("Caching SSL_CERT_FILE {}", cert_file);
debug!("Caching SSL_CERT_FILE {:?} -> {:?}",
cert_file, cert_cache_dir);
@smacfarlane smacfarlane requested a review from baumanj Sep 6, 2019
@baumanj
baumanj approved these changes Sep 6, 2019
@smacfarlane smacfarlane force-pushed the sm/cache-ssl-in-studio branch from 3621f62 to 1ba517b Sep 6, 2019
@smacfarlane smacfarlane requested a review from scotthain as a code owner Sep 6, 2019
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
@smacfarlane smacfarlane force-pushed the sm/cache-ssl-in-studio branch from b41371d to 4bd058b Sep 6, 2019
@smacfarlane smacfarlane merged commit 1fc6c4a into master Sep 6, 2019
5 checks passed
5 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #3356 passed (23 minutes, 1 second)
Details
buildkite/habitat-sh-habitat-master-website Build #473 passed (41 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@chef-ci chef-ci deleted the sm/cache-ssl-in-studio branch Sep 6, 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.