TLS integration and tests added

[blakehatch]

Description

Added tests to TLS integration from @allada

Towards: #301

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please also list any relevant details for your test configuration

Checklist

• Updated documentation if needed
• Tests added/amended
• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[MarcusSorealheis]

Reviewable status: 0 of 16 files reviewed, 1 unresolved discussion (waiting on @blakehatch)

---

.DS_Store line 0 at r1 (raw file):
Please add .DS_Store to .gitignore, maybe in a MacOS section, and remove it from the commit.

[MarcusSorealheis]

Reviewed 1 of 16 files at r1.
Reviewable status: 1 of 16 files reviewed, 2 unresolved discussions (waiting on @blakehatch)

---

cas/cas_main.rs line 413 at r1 (raw file):

                return Err(Box::new(make_err!(
                    Code::InvalidArgument,
                    "Expected 1 key in file {}, found {} keys",

Does it make sense to test this behavior? My intuition wants us to test for 0 keys or more than 1 key in the tls_config.key file.

I don't think it will ever happen, necessarily, but I want to watch for this regression. I also understand that there is cost with each added test. It does feel a bit like we are testing rustls.

[allada]

Reviewed 13 of 16 files at r1, 2 of 2 files at r2.
Reviewable status: 16 of 17 files reviewed, 24 unresolved discussions (waiting on @blakehatch and @MarcusSorealheis)

---

.gitignore line 9 at r2 (raw file):

.update_scheduler_ips.zip
__pycache__
.DS_Store

nit: Add new line at end of file.

---

run_integration_tests.sh line 102 at r2 (raw file):

done

echo "All tests passed!"

no change to file needed.

---

cas/grpc_service/bytestream_server.rs line 182 at r2 (raw file):

    }

    async fn create_or_join_upload_stream(

nit: This entire file has unrelated changes.

---

cas/scheduler/simple_scheduler.rs line 823 at r2 (raw file):

            }

            for awaited_action in inner.queued_actions.values() {

nit: This entire file has unrelated changes.

---

cas/store/s3_store.rs line 149 at r2 (raw file):

                DefaultCredentialsProvider::new().expect("failed to create credentials provider");

            // let region = config

nit: This entire file has unrelated changes.

---

cas/worker/running_actions_manager.rs line 865 at r2 (raw file):

                                    file_info
                                })
                                .err_tip(|| format!("Uploading file {full_path:?}"))?,

nit: This entire file has unrelated changes.

---

config/cas_server.rs line 228 at r2 (raw file):

    ///
    /// Default: None
    pub tls: Option<TlsConfig>,

nit: Needs #[serde(default)]

---

config/examples/basic_cas.json line 64 at r2 (raw file):

    }
  },
  // "workers": [{

nit: This entire file has unrelated changes.

---

integration_tests/simple_tls_test.sh line 2 at r2 (raw file):

#!/bin/bash
# Copyright 2022 The Turbo Cache Authors. All rights reserved.

nit: 2023

---

integration_tests/simple_tls_test.sh line 18 at r2 (raw file):

# This is an integration test to ensure TLS connections work properly.

# Extract TLS configuration using jq

nit: Comment is needless. Users can clearly read the code to see what it's doing.

---

integration_tests/simple_tls_test.sh line 19 at r2 (raw file):

# Extract TLS configuration using jq
TLS_CERT_FILE=$(jq -r '.servers[0].tls.cert_file' turbo_cache/config/examples/basic_cas.json)

nit: Needs a separate file. TLS should not be the preferred way. Users should use load balances for TLS support when able.

---

integration_tests/simple_tls_test.sh line 22 at r2 (raw file):

TLS_KEY_FILE=$(jq -r '.servers[0].tls.key_file' turbo_cache/config/examples/basic_cas.json)

# Define server and addresses (replace with your actual values)

nit: Comment is needless. Users can clearly read the code to see what it's doing.

---

integration_tests/simple_tls_test.sh line 33 at r2 (raw file):

set -x

# Run bazel

nit: Comment is needless. Users can clearly read the code to see what it's doing.

---

integration_tests/simple_tls_test.sh line 34 at r2 (raw file):

# Run bazel
bazel --output_base="$BAZEL_CACHE_DIR" test --config self_test //:dummy_test

We are not testing bazel, so no need to do this.

---

integration_tests/simple_tls_test.sh line 36 at r2 (raw file):

bazel --output_base="$BAZEL_CACHE_DIR" test --config self_test //:dummy_test

# Test a successful TLS connection to the success server

nit: Please end comments in a period.

---

integration_tests/simple_tls_test.sh line 37 at r2 (raw file):

# Test a successful TLS connection to the success server
# retry a few times as service may take a few seconds to get started

nit: Caps & period.

---

integration_tests/simple_tls_test.sh line 40 at r2 (raw file):

curl --retry 5 --retry-delay 0 --retry-max-time 30 --cert "$TLS_CERT_FILE" --key "$TLS_KEY_FILE" --insecure "$LISTEN_ADDRESS"

# Check if the connection was successful

nit: Period.

---

integration_tests/simple_tls_test.sh line 49 at r2 (raw file):

# Now, test an unsuccessful TLS connection to an invalid address
curl --cert "$TLS_CERT_FILE" --key "$TLS_KEY_FILE" --insecure "$INVALID_ADDRESS"

nit: Lets not test this, it doesn't really make sense to do this.

---

integration_tests/simple_tls_test.sh line 51 at r2 (raw file):

curl --cert "$TLS_CERT_FILE" --key "$TLS_KEY_FILE" --insecure "$INVALID_ADDRESS"

# Check if the connection was unsuccessful

nit: Period.

---

tools/cargo_shared.bzl line 163 at r2 (raw file):

        "version": "0.2.3",
    },
    "tls-listener": {

unused.

---

tools/cargo_shared.bzl line 170 at r2 (raw file):

        "version": "0.24.1",
    },
    "rustls-pemfile": {

unused.

---

tools/cargo_shared.bzl line 173 at r2 (raw file):

        "version": "1.0.3",
    },
    "rcgen": {

unused.

[allada]

Reviewed 6 of 7 files at r3.
Reviewable status: 15 of 17 files reviewed, 12 unresolved discussions (waiting on @blakehatch and @MarcusSorealheis)

---

cas/cas_main.rs line 413 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

Does it make sense to test this behavior? My intuition wants us to test for 0 keys or more than 1 key in the tls_config.key file.

I don't think it will ever happen, necessarily, but I want to watch for this regression. I also understand that there is cost with each added test. It does feel a bit like we are testing rustls.

Yeah, I dont think this is something we should test. Besides, we simply only support exactly 1 key, in the future we may support more, so no need to test more than 1. If it was less than 1, it'd fail to connect with curl.

---

cas/grpc_service/bytestream_server.rs line 556 at r3 (raw file):

        resp
    }
}

nit:needs new line at end of file.

[blakehatch]

@allada Is there a way you want me to test with other LBs like on GCP or Azure?

Also I'm not able to run the start_turbo_cache.sh script to start the terraform deployment example as I don't have the create_filesystem.sh file in my root directory. If you want to test out on your machine or if you can send me the file I do final verification the script is working.

[allada]

Reviewed 3 of 9 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: 19 of 23 files reviewed, 6 unresolved discussions (waiting on @blakehatch and @MarcusSorealheis)

---

cas/cas_main.rs line 431 at r11 (raw file):

        let tcp_listener = TcpListener::bind(&socket_addr).await?;
        let mut http = Http::new();
        http.http2_keep_alive_interval(Duration::from_secs(10));

nit: Remove this, it's not part of this PR.

---

cas/cas_main.rs line 432 at r11 (raw file):

        let mut http = Http::new();
        http.http2_keep_alive_interval(Duration::from_secs(10));
        http.http2_max_pending_accept_reset_streams(1000);

nit: Remove this, it's not part of this PR.

---

deployment-examples/docker-compose/key.pem line 1 at r11 (raw file):

-----BEGIN PRIVATE KEY-----

nit: Can we rename these example-do-not-use-in-prod-key.pem and same with cert... We don't want people to use these keys, they are for demo only.

---

deployment-examples/docker-compose/local-storage-cas.json line 74 at r11 (raw file):

  },
  {
    "listen_address": "0.0.0.0:50071",

nit: Put a line here saying:
// Example of serving over TLS on port 50071.

[MarcusSorealheis]

@blakehatch I will approve after you address the two open comments

[MarcusSorealheis]

There's also an awful lot of test failures.

[blakehatch]

Reviewable status: 16 of 25 files reviewed, 2 unresolved discussions (waiting on @allada and @MarcusSorealheis)

[MarcusSorealheis]

@aaronmondal Any idea why the CI is failing? Specifically, Nix Bazel?

[allada]

CI is failing because .into() needs to be removed. If you run bazel test //... it will tell you everything:

ERROR: /home/runner/work/turbo-cache/turbo-cache/cas/BUILD:3:12: Clippy cas/cas.clippy.ok failed: (Exit 1): process_wrapper failed: error executing command (from target //cas:cas) bazel-out/k8-opt-exec-2B5CBBC6/bin/external/rules_rust/util/process_wrapper/process_wrapper --arg-file bazel-out/k8-fastbuild/bin/external/crate_index__axum-0.6.19/axum_build_script.linksearchpaths ... (remaining 322 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
error: useless conversion to the same type: `std::vec::Vec<u8>`
   --> cas/cas_main.rs:420:53
    |
420 |                 .with_single_cert(certs, PrivateKey(keys.into_iter().next().unwrap().into()))
    |                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `keys.into_iter().next().unwrap()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
    = note: `-D clippy::useless-conversion` implied by `-D warnings`

error: aborting due to previous error

[aaronmondal]

Reviewed 2 of 9 files at r12, 8 of 8 files at r13, all commit messages.
Reviewable status: 19 of 23 files reviewed, 5 unresolved discussions (waiting on @allada, @blakehatch, and @MarcusSorealheis)

---

-- commits line 2 at r13:
nit: You could shorten this to "Add TLS integration". No need to mention the tests 😇

---

cas/cas_main.rs line 421 at r13 (raw file):

                .with_safe_defaults()
                .with_no_client_auth()
                .with_single_cert(certs, PrivateKey(keys.into_iter().next().unwrap().into()))

The tests are failing because the bazel tests use clippy which isn't enabled for the Cargo Build. The error is this:

error: useless conversion to the same type: `std::vec::Vec<u8>`
   --> cas/cas_main.rs:421:53
    |
421 |                 .with_single_cert(certs, PrivateKey(keys.into_iter().next().unwrap().into()))
    |                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `keys.into_iter().next().unwrap()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
    = note: `-D clippy::useless-conversion` implied by `-D warnings`

error: aborting due to previous error

The fix is to remove the into() as the message says.

I'll try to get better integration for clippy for the Cargo build.

---

integration_tests/simple_tls_test.sh line 14 at r13 (raw file):

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

nit: Missing empty line.

[MarcusSorealheis]

Reviewable status: 18 of 23 files reviewed, 4 unresolved discussions (waiting on @aaronmondal, @allada, and @blakehatch)

---

cas/cas_main.rs line 421 at r13 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

The tests are failing because the bazel tests use clippy which isn't enabled for the Cargo Build. The error is this:

error: useless conversion to the same type: `std::vec::Vec<u8>`
   --> cas/cas_main.rs:421:53
    |
421 |                 .with_single_cert(certs, PrivateKey(keys.into_iter().next().unwrap().into()))
    |                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `keys.into_iter().next().unwrap()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
    = note: `-D clippy::useless-conversion` implied by `-D warnings`

error: aborting due to previous error

The fix is to remove the into() as the message says.

I'll try to get better integration for clippy for the Cargo build.

Done

[MarcusSorealheis]

Reviewable status: 18 of 23 files reviewed, 2 unresolved discussions (waiting on @aaronmondal, @allada, and @blakehatch)

[MarcusSorealheis]

Reviewable status: 18 of 23 files reviewed, 1 unresolved discussion (waiting on @aaronmondal and @allada)

[aaronmondal]

Reviewed 1 of 1 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: 19 of 23 files reviewed, all discussions resolved (waiting on @allada and @MarcusSorealheis)