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

[k8s] Add all root CA certs to TLS connections #3616

Conversation

darobs
Copy link
Contributor

@darobs darobs commented Sep 26, 2020

There were a few places in the code where we were not reading the root CA data correctly. It is possible to have multiple certificates in the file/data, and we were only extracting the first one.

The three places where we need to consider the possibilities of multiple root CAs:

  1. iotedged-proxy. iotedged mounts its trusted CAs for proxy to use.
  2. When reading the KubeConfig context's certificate_authority
  3. When reading the serviceaccount/ca.crt

Copy link
Contributor

@dmolokanov dmolokanov left a comment

Choose a reason for hiding this comment

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

Some nits and a couple of questions

edgelet/edgelet-http-workload/tests/dns-san.rs Outdated Show resolved Hide resolved

// Extract each certificate's string. The final string from the split will either be empty
// or a non-certificate entry, so it is dropped.
let delimiter = "-----END CERTIFICATE-----";
Copy link
Contributor

Choose a reason for hiding this comment

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

this parsing looks a bit sketchy. should we use OpenSSL load the chain?

edgelet/iotedge-proxy/src/proxy/config.rs Outdated Show resolved Hide resolved
@@ -18,3 +18,109 @@ pub mod kube;
pub use self::client::{Client, HttpClient};
pub use self::config::{get_config, Config, TokenSource, ValueToken};
pub use self::error::{Error, ErrorKind, RequestType};

#[cfg(test)]
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

should we remove that?

Copy link
Contributor Author

@darobs darobs Sep 30, 2020

Choose a reason for hiding this comment

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

There is some dead code I wound up not using for my tests. I figured it would be more valuable to leave in the CertGenerator options.

self
}

pub fn generate(&self) -> Result<Vec<u8>, CertGeneratorError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, we have cert generator! I may steal it for the mqtt broker 😉

Copy link
Contributor

@dmolokanov dmolokanov left a comment

Choose a reason for hiding this comment

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

:shipit:

@darobs darobs merged commit 518fe7a into Azure:edge-on-k8s-public-preview Sep 30, 2020
@darobs darobs deleted the darobs/trust-bundle-may-have-multiple-certs branch September 30, 2020 17:33
darobs added a commit that referenced this pull request Oct 15, 2020
Changes merged into beta11:
[k8s] Set resource limits and requests for Edge runtime components (#3666)
[k8s public preview] Have EdgeDeploymentOperator report status to Agent (#3232)
[k8s] Update Helm charts to allow for correct proxy settings. (#3641)
[k8s] Add all root CA certs to TLS connections (#3616)
[k8s] Allow Edge on K8s modules to join HostNetwork. (#3618)
darobs added a commit to darobs/iotedge that referenced this pull request Dec 4, 2020
* Add all root CA certs to TLS connections

* Use openssl stack_from_pem to parse trusted roots
darobs added a commit to darobs/iotedge that referenced this pull request Dec 4, 2020
Changes merged into beta11:
[k8s] Set resource limits and requests for Edge runtime components (Azure#3666)
[k8s public preview] Have EdgeDeploymentOperator report status to Agent (Azure#3232)
[k8s] Update Helm charts to allow for correct proxy settings. (Azure#3641)
[k8s] Add all root CA certs to TLS connections (Azure#3616)
[k8s] Allow Edge on K8s modules to join HostNetwork. (Azure#3618)
darobs added a commit that referenced this pull request Dec 17, 2020
This rebases all changes from Edge on K8s public preview branch into latest release/1.0.10 branch.
Cherry-picked all commits from merge base to latest edge-on-k8s-public-preview branch into this PR, then cleaned it up.
Most C# code is in the "Kubernetes" project, but some changes affect the "Service" and "Core" projects.

Tests:

TEST:
Bring up k8s based on: https://microsoft.github.io/iotedge-k8s-doc/
only using the images from this PR's build.

[k8s] Update Helm charts to allow for correct proxy settings. (#3641)
TEST:
Set ".Values.iotedged.data.no_proxy" on installation.
Confirm "no_proxy" environment var is set for iotedged and agent pods.

[k8s] Add all root CA certs to TLS connections (#3616)
TEST:
Set up Edge runtime with self-signed certificate. Ensure it starts and all modules connect.

[k8s] Allow Edge on K8s modules to join HostNetwork. (#3618)
TEST:
Set "createOptions.NetworkMode=Host" in a module, ensure it starts and connects to EdgeHub.

[k8s] Set resource limits and requests for Edge runtime components #3666
TEST:
Start runtime with these Helm charts. Once pods are running, run
kubectl get pod -n -o yaml
For each pod.
Confirm iotedged has resource limit & request set. and qosClass: Guaranteed
Confirm edgagent has resource limit for all containers and qosClass: Guaranteed
Module will have a resource limit for proxy container, and may or may not have resource for module.

[k8s public preview] Have EdgeDeploymentOperator report status to Agent #3232
TEST:

Set up a module with bad bind mount:
{"image":"mcr.microsoft.com/media/live-video-analytics:1","createOptions":{"hostConfig":{"Binds":["/home/lvaadmin/samples/output:/var/media/","//:/var/lib/azuremediaservices/"],"LogConfig":{"Config":{"max-size":"10m","max-file":"10"},"Type":""}}},"auth":null}

Add space at the end of the ACR user name in the deployment. Use a module that attempts to access the ACR.

Both setups should not crash agent. Runtime status of device on portal should show a "500" error.

[k8s] Discovered some problems with createOptions parsing. (#3743)
TEST:
Set a module with Cmd, EntryPoint, WorkingDir with different capitalization and ensure these are parsed and assigned in pod.

[k8s] Environment Variables can be null or empty. (#3780)
TEST:
In Agent section of Helm charts, set an empty environment variable, ensure iotedged starts and launches Agent with empty variable.

In any module's environment variable section, make an unset environment variable, and one that is all "=", ensure Pod is created with these environment variables present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants