Skip to content
This repository has been archived by the owner on Dec 16, 2020. It is now read-only.

wasm abi: tls certificate information retrieval #476

Closed
wants to merge 3 commits into from

Conversation

Shikugawa
Copy link
Member

Signed-off-by: shikugawa Shikugawa@gmail.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: This is only a proposal implementation. I added instructions to retrieve local/peer certificate information. It is required to retrieve on AuthN filter wasm implementation. If there is no implementation to retrieve cert info via wasm VM, we should link upstream envoy connection info. It may cause the complexity of dependencies of AuthN wasm implementation.

istio/istio#15772

Risk Level: Low
Testing: N/A
Docs Changes: Required
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: shikugawa <Shikugawa@gmail.com>
@Shikugawa Shikugawa changed the title wasm abi: tls certificate informations retrieval wasm abi: tls certificate information retrieval Apr 6, 2020
@PiotrSikora
Copy link
Contributor

@Shikugawa thanks for the PR, but could you send a PR with the ABI changes to https://github.com/proxy-wasm/spec, or just create an issue there to discuss this feature?

Ultimately, I don't think that we need to have a separate function for each buffer/map, and this could be exposed either as a raw certificate via proxy_get_buffer(TlsCertificate{Local,Peer}, ...) or as a map with extracted values via proxy_get_map(TlsCertificate{Local,Peer}, ...).

cc @jplevyak

@kyessenov
Copy link
Contributor

kyessenov commented Apr 6, 2020

Why do we need this if we can use getValue({"connection", "subject_local_certificate"}, &value) from https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/security/rbac_filter#condition?

@Shikugawa
Copy link
Member Author

@kyessenov If we use this, we must build rbac_filter with emscripten. But we'd not like to do what the number of dependencies related to upstream envoy will grow up. So we need this functionality.

@kyessenov
Copy link
Contributor

@Shikugawa I don't follow. getValue lets you add more getters without bloating the API and we already added a bunch for TLS info for telemetry. There's no extra dependencies on rbac filter, it just uses the underlying code to match the property names.

@Shikugawa
Copy link
Member Author

@kyessenov Thanks. I'll try it with this. But, this functionality may be required in the future. So I think that continuing a discussion with this is useful.

@kyessenov
Copy link
Contributor

kyessenov commented Apr 6, 2020

Sure, please take a look and we can decide how to grow the set of the attributes. I just don't want two different ways to get the same things.

Signed-off-by: shikugawa <Shikugawa@gmail.com>
Signed-off-by: shikugawa <Shikugawa@gmail.com>
@Shikugawa
Copy link
Member Author

@kyessenov I tried it with getValue. But it has less functionality to solve our problem such as we can't retrieve whether peer certificate is presented. So still we need this.

@kyessenov
Copy link
Contributor

@Shikugawa See connection.mtls: Indicates whether TLS is applied to the downstream connection and the peer ceritificate is presented. This is not entirely possible on the client by the way, there is my long failed attempt to implement it in envoy repo.

@@ -80,9 +80,10 @@ Word set_effective_context(void* raw_context, Word context_id);
Word done(void* raw_context);
Word call_foreign_function(void* raw_context, Word function_name, Word function_name_size,
Word arguments, Word warguments_size, Word results, Word results_size);
Word get_peer_certificate_info(void* raw_context, Word value_ptr_ptr, Word value_size_ptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding this as a core ABI, let's add it to getProperties.

@Shikugawa Shikugawa deleted the get-connection-abi branch December 16, 2020 03:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants