Add TLS support to the ctl gateway #7923

davidMcneil · 2020-09-18T14:32:07Z

This PR adds TLS support to the ctl gateway. Below is an explanation of the general UI/UX around this feature. There is still work to be done on the overall workflow. Specific questions/pain points are called out.

Generate a Self Signed Key and Certificate

Users can use their own keys and certificates, but the most straightforward option is to let the Habitat CLI generate a private key and self signed certificate. This can be done with the command hab sup secret generate-tls --subject-alternative-name <dns name>. This will automatically create two files ctl-gateway-<timestamp>.crt.pem and ctl-gateway-<timestamp>.key.pem storing them in /hab/cache/keys/ctl by default.

Questions

Where should this command live hab sup secret generate-tls was used because we already had hab sup secret generate for creating the ctl secret token, but it does not feel like quite the right place.
Is /hab/cache/keys/ctl a good place to store the keys and certs

Start the Supervisor with TLS Enabled

This PR adds three new flags to the hab sup run command.

--ctl-client-ca-certificate <CTL_CLIENT_CA_CERTIFICATE>
    Enable client authentication for the control gateway and set the certificate authority to use when
    authenticating the client [default: /hab/cache/keys/ctl]
--ctl-server-certificate <CTL_SERVER_CERTIFICATE>
    The control gateway server certificate to use for TLS [default: /hab/cache/keys/ctl
--ctl-server-key <CTL_SERVER_KEY>
    Enable TLS for the control gateway and set the private key
    
    See `ctl-server-certificate` and `ctl-client-certificate` for additional settings. [default:
    /hab/cache/keys/ctl]

If these paths are pointed at a directory (eg /hab/cache/keys/ctl by default) they will use the latest file matching the ctl-gateway-<timestamp>.crt|key.pem format. If these paths point to a file --ctl-client-ca-certificate and --ctl-server-certificate will try to read PEM formatted certificates and --ctl-server-key will try to read PEM, PKCS8 private keys. If --ctl-client-ca-certificate is not specified no client authentication will be used.

Connect with the Habitat CLI Client

This is currently the least polished part of the process. All configuration is accomplished through three environment variables
HAB_CTL_GATEWAY_CLIENT_CERTIFICATE, HAB_CTL_GATEWAY_CLIENT_KEY HAB_CTL_GATEWAY_SERVER_CA_CERTIFICATE these variables are the client-side equivalent of the hab sup run CLI arguments. Environment variables are used, as opposed to CLI flags, due to the large number of commands requiring these options.

This PR now adds five fields to cli.toml instead of using environment variables: listen_ctl, ctl_client_certificate, ctl_client_key, ctl_server_ca_certificate, and ctl_server_name_indication.

Questions
3. How should we allow these options to be set? Leave the environment variables, add CLI options, use a config file as we do with /hab/sup/default/CTL_SECRET
4. Currently disabling server authentication is not possible. Would that be a desired feature?
5. Should we add logic to automatically detect the server is using TLS and upgrade the client appropriately? Or should we require the user to specify that a TLS connection should be used?

Example with both Client and Server Authentication

# Generate new keys
> hab sup secret generate-tls --subject-alternative-name localhost
# Start the server with TLS using the default values (ie the key and cert we just generated in `/hab/cache/keys/ctl`)
> hab sup run --ctl-server-certificate --ctl-server-key --ctl-client-ca-certificate
# Start the client with the defaults and get the service status. Note setting `--remote-sup` to `localhost` is important. It must match the `--subject-alternative-name` we used when generating the key
> env HAB_CTL_GATEWAY_CLIENT_CERTIFICATE=/hab/cache/keys/ctl \
  env HAB_CTL_GATEWAY_CLIENT_KEY=/hab/cache/keys/ctl \
  env HAB_CTL_GATEWAY_SERVER_CA_CERTIFICATE=/hab/cache/keys/ctl \
  hab sup status --remote-sup localhost

Example with only Server Authentication

# Generate new keys
> hab sup secret generate-tls --subject-alternative-name localhost
# Start the server with TLS using the default values (ie the key and cert we just generated in `/hab/cache/keys/ctl`)
> hab sup run --ctl-server-certificate --ctl-server-key
# Start the client with the defaults and get the service status. Note setting `--remote-sup` to `localhost` is important. It must match the `--subject-alternative-name` we used when generating the key
> env HAB_CTL_GATEWAY_SERVER_CA_CERTIFICATE=/hab/cache/keys/ctl \
  hab sup status --remote-sup localhost

Other Questions
6. Currently the traffic sent over the TLS connection is identical to what is sent when not using TLS. Specifically, we still send the CTL_SECRET and do our custom handshake. How should we handle this?

TODO

Refine the UI/UX
Investigate all error conditions and ensure the error messages are reasonably helpful
Add testing

christophermaier · 2020-09-18T15:10:02Z

components/sup/Cargo.toml

@@ -47,9 +47,7 @@ prost-types = "*"
 rand = "*"
 rants = { version = "*", features = ["native-tls"] }
 regex = "*"
-# Pinning for now. Since upgrading to 0.17.0 results in conflicts with other crates
-# See https://github.com/habitat-sh/habitat/issues/7523


I guess we'll be able to close this issue, then?

christophermaier · 2020-09-18T15:44:01Z

components/core/src/tls/rustls_reader.rs

+               Cursor},
+          path::{Path,
+                 PathBuf}};
+use thiserror::Error;


christophermaier · 2020-09-18T15:56:18Z

components/core/src/tls/rustls_reader.rs

+        return Err(Error::FailedToReadCertificatesFromRootStore(failed));
+    }
+    Ok(root_certificate_store)
+}


The various X_from_buf functions look like they could be private, and private_key_from_buf appears unused.

private_keys_from_file also looks like it could be private.

It seems like it would also be better to use BufReader instead of Cursor for these functions. Using fs::read will pull the contents of the entire file into memory, which we don't really need to do in this case. I think Cursor is mainly useful for taking data that already lives in memory, but adapting it for the kind of operations of BufRead. Since we don't already have this in memory, but have to read it off disk anyway, this seems like an unnecessary complication.

Thanks! Addressed in a0e8d3b

christophermaier · 2020-09-18T16:16:18Z

components/core/src/tls/tcp_or_tls_stream.rs

+
+    /// Upgrade a `TcpStream` into a `TlsStream` using server configuration
+    pub async fn to_tls_server(self, tls_config: Arc<TlsServerConfig>) -> io::Result<Self> {
+        Ok(match self {


Given that this match block is so big, I think it might be a little more clear to compute the new value, and then have Ok(stream) be the final line of the function.

Thanks! Addressed in 28bd76a

christophermaier · 2020-09-18T16:26:38Z

components/core/src/tls/tcp_or_tls_stream.rs

+                                              .map_err(|e| e.0)?;
+                Self::TlsStream(TlsStream::Server(tls_stream))
+            }
+            Self::TlsStream(stream) => Self::TlsStream(stream),


I wonder if having a stream.is_tls() function might be nice here. That could make it a little more clear that this branch is really returning the incoming TLS stream unchanged.

My main motivation here is that TlsStream and TcpStream look very orthographically similar at a glance, so taking pains to make it incredibly clear what's going on could be a big win.

if self.is_tls() { Ok(self) } else { // do stuff Ok(upgraded) }

Another thought: would it ever be plausible to want to change the TLS configuration of an existing TLS stream? Would it be better to call this function maybe_upgrade_to_tls? 🤔

I wonder if having a stream.is_tls() function might be nice here

I think we need to do some type of matching/destructuring in order to access the interior stream in the non-tls case. I could use if let here, but I generally prefer match in these situations so the compiler will direct future developers to conditions based on the enum variant.

My main motivation here is that TlsStream and TcpStream look very orthographically similar at a glance

Agreed, maybe RawTcpStream and TlsTcpStream would be better names?

Another thought: would it ever be plausible to want to change the TLS configuration of an existing TLS stream? Would it be better to call this function maybe_upgrade_to_tls?

Probably not. I made those methods private and added the maybe_ prefix which I agree makes it more clear.

Changes in 28bd76a

christophermaier · 2020-09-18T16:51:23Z

components/hab/src/cli/hab/util/tls.rs

+        };
+        Ok(Self { path, private_key })
+    }
+}


Similar comments re: FromStr vs TryFrom<Path> apply here, as well.

Same explanation as #7923 (comment)

christophermaier · 2020-09-18T16:51:43Z

components/hab/src/cli/hab/util/tls.rs

+    type Error = habitat_core::Error;
+
+    fn try_from(s: &str) -> Result<Self, Self::Error> { Self::from_str(s) }
+}


Same comments apply.

Same explanation as #7923 (comment)

christophermaier · 2020-09-18T16:51:52Z

components/hab/src/cli/hab/util/tls.rs

+
+impl From<PrivateKey> for String {
+    fn from(pkg_ident: PrivateKey) -> Self { pkg_ident.to_string() }
+}


Same comments apply.

Same explanation as #7923 (comment)

christophermaier · 2020-09-18T16:58:17Z

components/hab/src/cli/hab/util/tls.rs

+}
+
+impl RootCertificateStore {
+    pub fn root_certificate_store(self) -> RootCertStore { self.root_certificate_store }


This might be a bit more idiomatic as into_inner, or perhaps just an Into or From implementation.

We're basically using RootCertificateStore as a way to get information from the user interacting with our CLI down to the point in the code when we can start to actually use the underlying TLS machinery. Really, it's just a way to tie a file path from the user to a certificate store.

Viewed in that light, maybe RootCertificateStore is actually just RootCertificateStore(PathBuf), which then has a From<RootCertificateStore> for RootCertStore implementation.

This same reasoning applies to the PrivateKey type above, as well.

This might be a bit more idiomatic as into_inner

Agreed. Addressed in 4dae869

Viewed in that light, maybe RootCertificateStore is actually just RootCertificateStore(PathBuf), which then has a From<RootCertificateStore> for RootCertStore implementation.

This is certainly possible and I agree makes more sense. However, by having RootCertificateStore parse the file we can catch errors as part of CLI parsing. If RootCertificateStore only had to be a valid PathBuf that would require parsing that PathBuf deep in the code and then bubbling that error back up. I would rather fail early, but I think there are arguments to be made for both approaches.

christophermaier · 2020-09-18T17:03:08Z

components/sup-client/src/lib.rs

@@ -141,24 +144,35 @@ impl SrvClient {
        secret_key: &str,
        request: impl Into<SrvMessage> + fmt::Debug)
        -> Result<impl Stream<Item = Result<SrvMessage, io::Error>>, SrvClientError> {
-        let socket = TcpStream::connect(address.as_ref()).await?;
-        let mut socket = Framed::new(socket, SrvCodec::new());
+        let tcp_stream = TcpStream::connect(address.as_ref()).await?;


Good... tcp_stream is a better variable name than socket 👍

stevendanna · 2020-09-22T17:33:03Z

components/core/src/tls/ctl_gateway.rs

+                                                -> Result<(), Error> {
+    let mut params = CertificateParams::new(vec![subject_alternate_name.to_string(),
+                                                 "localhost".to_string(),
+                                                 "127.0.0.1".to_string(),


I'm not sure it will matter for our use here, but one strange thing about this library is that it assumes all subject alt names are dnsNames:

X509v3 Subject Alternative Name: DNS:foo, DNS:localhost, DNS:127.0.0.1, DNS:0.0.0.0

Ideally, IP addresses would get ip address entries there.

Thanks, nice catch! I should remove the IP addresses. The tokio_rustls library does not allow using an IP address when specifying the domain. As a result, adding those IP addresses does not buy us anything.

I imagine the implication then is also that users who want to use this feature for remote supervisors will have to use a hostname rather than an IP to connect.

One nice "development" feature you might consider is allowing the client to specify the server name via some option that then gets passed through to the SNI we send from the client. This would allow doing small tests where you specify the IP address as the host to connect to while still allowing the hostname verification to work. It isn't a big deal but can be helpful sometime for situation where you can't get DNS to work out consistently for one reason or another.

Agreed, that is a nice feature. The field ctl_server_name_indication in cli.toml allows setting this.

netlify · 2020-09-22T18:15:34Z

Deploy preview for chef-habitat processing.

Building with commit 86c97bc

https://app.netlify.com/sites/chef-habitat/deploys/5f7643c9231ed90007085964

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

christophermaier · 2020-09-30T17:41:57Z

components/core/src/tls/ctl_gateway.rs

+    let mut params =
+        CertificateParams::new(vec![subject_alternate_name.to_string(), "localhost".to_string(),]);
+    let mut distinguished_name = DistinguishedName::new();
+    distinguished_name.push(DnType::CommonName, "Habitat Supervisor Control Gateway");


I thought a CommonName had to be a FQDN or wildcard domain name?

Nice catch! I switched to use DynType::Organization. Addressed in 34a7f27

It's odd it looks like changing it does not actually change the cert. The default value uses an invalid common name https://github.com/est31/rcgen/blob/ac4813acbbe90c458fbb016b5a08631edecabeca/src/lib.rs#L458

christophermaier · 2020-09-30T17:48:44Z

components/core/src/tls/ctl_gateway.rs

+                                                path: impl AsRef<Path>)
+                                                -> Result<(), Error> {
+    let mut params =
+        CertificateParams::new(vec![subject_alternate_name.to_string(), "localhost".to_string(),]);


I think the subject alternate name is supposed to be a valid DNS name... do we check for that anywhere?

Agreed. Addressed in 6e5fa2a

christophermaier · 2020-09-30T17:51:43Z

components/core/src/tls/ctl_gateway.rs

+    Ok(())
+}
+
+fn get_latest_path(path: impl AsRef<Path>, pattern: &str) -> Result<PathBuf, Error> {


Here, path is really a directory, while pattern describes files within that directory, correct?

If so, it would be nice to have the variable names reflect that, along with some documentation comments.

Addressed in 86c97bc

christophermaier · 2020-09-30T18:06:39Z

components/core/src/tls/ctl_gateway.rs

+    Ok(rustls_wrapper::private_key_from_file(&path)?)
+}
+
+pub fn latest_root_certificate_store(path: impl AsRef<Path>) -> Result<RootCertStore, Error> {


The fact that these functions are in this module, but are called from tls::rustls_wrapper::types and call functions from tls::rustls_wrapper themselves suggests that maybe things are not located in the right place in the module hierarchy.

Over in tls::rustls_wrapper::types, the only thing that seems to distinguish things is whether something is called with a directory or a file path, whereas over here, it seems like "control gateway-ness" of the files is the distinguishing feature, which seem at odds with each other.

(On the other hand, generate_self_signed_certificate_and_key definitely pertains to the control gateway, so its location here makes complete sense.)

Is there a better organization that would help clarify this?

christophermaier · 2020-09-30T18:33:38Z

components/hab/src/cli/hab/sup.rs

          net::{IpAddr,
                SocketAddr},
          path::PathBuf,
          str::FromStr};
 use structopt::{clap::AppSettings,
                StructOpt};

+const HAB_CTL_KEYS_CACHE: &str = "/hab/cache/keys/ctl";


We might want to put this into habitat_core::fs for consistency.

Addressed in 4ed067a

christophermaier · 2020-09-30T18:47:18Z

components/sup-client/src/lib.rs

+            Some(Arc::new(tls_config))
+        } else {
+            None
+        };


🤔 Rather than having this function implicitly depend on CliConfig, it might be more clear to pass that in as an argument.

Possibly better would be to give CliConfig a function that could generate the necessary TLS config object, and then pass that into this function.

🤔 this is a difficult question; I wrestled with it as well. I actually made the opposite decision and removed the secret parameter from the request function. My reasoning was that whenever request was called we did the exact same thing: lookup the secret and then pass it to the function. This simply complicates the logic when we go to call request. Looking up the TLS parameters would be the same story. I agree it does not feel 100% correct to encapsulate this logic inside request. However, I do think we want it encapsulated somewhere. I am inclined to leave it in request for now and if we have a need for a more general request function we can make the change then. What do you think?

Maybe we could split the difference... still depend implicitly on CliConfig within the function, but encapsulate the TLS parameter creation as a method on CliConfig. If that's not too gnarly, that seems like a more natural location for the logic than here (similar arguments might be made for the server TLS config).

If that looks gross, though, we could leave it as is for the time being.

Yep, that sounds like a good approach. Addressed in 91a106e

christophermaier · 2020-09-30T18:50:52Z

components/sup/src/ctl_gateway/server.rs

+            Some(Arc::new(tls_config))
+        } else {
+            None
+        };


Similarly, it might be better for the CtlGatewayServer to be able to take an optional TlsServerConfig, rather than having the logic for creating that embedded here.

Addressed in 8643bcf

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

stevendanna · 2020-09-30T10:03:17Z

components/core/src/tls/ctl_gateway.rs

+    params.distinguished_name = distinguished_name;
+    params.alg = &PKCS_ECDSA_P256_SHA256;
+
+    let certificate = RcgenCertificate::from_params(params)?;


For the current use this likely doesn't matter since we also control the client, but I noted that this library uses a fixed Serial Number if you don't provide one:

Serial Number: 42 (0x2a)

we've hit problems related to reused serial numbers in various browsers. I've wrote a few words about it here:

https://github.com/chef/automate/blob/b42fa46102d7fb0aad9bdfb236e16d40ccf55e05/api/config/deployment/init_config.go#L354-L369

Might be worth fixing just in case this function gets reused for other purposes.

In the past we've also hit problems with self-signed certificates where the CA:TRUE basic constraint wasn't set (setting this constraint also requires adding KeyUsageCertSign to the key usage extensions. However, we also have a lot of self-sgined certs out in the wild that don't set this and haven't hit a problem so perhaps most of the clients that cared about this aren't an issue.

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

christophermaier

Nice work!

…tls" This reverts commit 0f03c89, reversing changes made to e3d33d0. This is a temporary action, pending some investigation of e2e test failures.

Revert "Merge pull request #7923 from habitat-sh/dmcneil/ctl-gateway-…

@davidMcneil

This is a second attempt at merging #7923. All changes are by @davidMcneil; I'm just the messenger. This reverts commit 5494773, reversing changes made to 59e3c3b. Signed-off-by: David McNeil <mcneil.david2@gmail.com> Signed-off-by: Christopher Maier <cmaier@chef.io>

Per [the migration guide][1] this wraps `AppState` as `Data<AppState>` to prevent panics in the HTTP gateway. The rest of the code currently assumes the data is returned in this way; future refactorings may allow us to drop the `Data` wrapper entirely, but this is the minimal change for now. Actix 3.0 was introduced in #7949 / #7923. [1]: https://github.com/actix/actix-web/blob/master/MIGRATION.md#300 Signed-off-by: Christopher Maier <cmaier@chef.io>

chef-expeditor bot assigned davidMcneil Sep 18, 2020

christophermaier reviewed Sep 18, 2020

View reviewed changes

stevendanna reviewed Sep 22, 2020

View reviewed changes

davidMcneil added 22 commits September 29, 2020 09:20

Update rants library

a89f160

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

[Refactor] Factor out TLS certificate fetching code

98ec45d

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Update rustls and actix-web

7618e2d

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add TLS to ctl gateway server

7f58018

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add TLS support to the ctl gateway client

a68c167

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add ca to certificate names

1d54800

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add hab sup secret generate-key command

bf4bc41

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

[Refactor] Remove uneeded error type

b8f5c67

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

[Refactor] Cleanup rustls readers

4dfa50d

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

[Refactor] PR feedback

75e5ba7

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

[Refactor] Cleanup TLS CLI types

cca89de

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Remove ip addresses from ctl gateway certs

175f169

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Update clap

7e2c864

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

[Refactor] Add some logging

01ec40d

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add ctl gateway tls file generation tests

6564e67

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Cleanup rustls readers

142013c

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Remove CtlSecretNotFound error

c2a9acf

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add TLS handshake failed response from ctl gateway server

789859e

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Send net error only when TLS handshake was not attempted

8ec85cb

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add option to ctl gateway to specify server name indication

b69d679

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Improve CLI messages

b40cb1d

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Improve CLI type documentation

f6b9bfd

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

davidMcneil force-pushed the dmcneil/ctl-gateway-tls branch from ec26844 to f6b9bfd Compare September 29, 2020 15:46

davidMcneil added 3 commits September 29, 2020 11:17

Add listen_ctl to cli.toml

fd35812

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add TLS client config to cli.toml

65ee8cb

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Add ctl gatew tls tests

cd660b1

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

Factor out '/hab/cache/keys/ctl' constant

661956d

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

davidMcneil marked this pull request as ready for review September 29, 2020 19:31

davidMcneil requested review from eeyun and smacfarlane as code owners September 29, 2020 19:31

Fix ps script analyzer

f35193a

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

christophermaier reviewed Sep 30, 2020

View reviewed changes

[Feedback] Use organization name

34a7f27

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

stevendanna reviewed Oct 1, 2020

View reviewed changes

davidMcneil added 5 commits October 1, 2020 14:32

[Feedback] Extract CliConfig::maybe_tls_client_config

91a106e

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

[Feedback] Extract CtlGatewayServer::maybe_tls_config

8643bcf

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

[Feedback] Move HAB_CTL_KEYS_CACHE to habitat_core::fs

4ed067a

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

[Feedback] Type check the subject alternative name

6e5fa2a

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

[Feedback] Add comment to get_last_path

86c97bc

Signed-off-by: David McNeil <mcneil.david2@gmail.com>

christophermaier approved these changes Oct 2, 2020

View reviewed changes

christophermaier merged commit 0f03c89 into master Oct 15, 2020

christophermaier added a commit that referenced this pull request Oct 16, 2020

Merge pull request #7943 from habitat-sh/cm/revert-tls

5494773

Revert "Merge pull request #7923 from habitat-sh/dmcneil/ctl-gateway-…

christophermaier mentioned this pull request Oct 23, 2020

Revert "Merge pull request #7943 from habitat-sh/cm/revert-tls" #7949

Merged

christophermaier mentioned this pull request Oct 23, 2020

[BUG] Update actix-web calling conventions #7950

Merged

Add TLS support to the ctl gateway #7923

Add TLS support to the ctl gateway #7923

Conversation

davidMcneil commented Sep 18, 2020 • edited Loading

Generate a Self Signed Key and Certificate

Start the Supervisor with TLS Enabled

Connect with the Habitat CLI Client

Example with both Client and Server Authentication

Example with only Server Authentication

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidMcneil Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidMcneil Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

stevendanna Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophermaier left a comment

Choose a reason for hiding this comment

davidMcneil commented Sep 18, 2020 •

edited

Loading

davidMcneil Sep 21, 2020 •

edited

Loading

davidMcneil Sep 22, 2020 •

edited

Loading

stevendanna Sep 23, 2020 •

edited

Loading

netlify bot commented Sep 22, 2020 •

edited

Loading