[API Review] Add Windows Store in the TLS validation context

[davinci26]

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com
Commit Message:
See #13596

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15291 was opened by davinci26.

see: more, trace.

[davinci26]

at this context the value for trusted_stores could be "ROOT", "CA" and other windows stores. see docs

[davinci26]

cc @mattklein123 @envoyproxy/windows-dev

[davinci26]

I wrapped the CaStore into a message instead of a string to be more extensible in the future

[mattklein123]

Can you add message docs for this also?

[davinci26]

For sure. Do you mind if add the message docs along with the implementation? Or do we want the docs for the api review

[mattklein123]

The API is simple. Once we have consensus I would just do it all in this PR?

[davinci26]

Yeah I will do everything in a single PR. Just deferring resolving this comment until a commit or two down the line when I have the implementation.

[mattklein123]

In general this looks reasonable to me. @lizan @ggreenway any thoughts from a TLS API perspective?

/wait

[mattklein123]

Can you add message docs for this also?

[davinci26]

@lizan @ggreenway let me know what you think, I am hoping to have this change land on 1.18

[ggreenway]

If this is the expected way to get trusted CAs on the platform, it seems fine. Should it be named windows-something, or are doc comments good enough?

Is there any facility to update a running envoy if the trust store changes, similar to SDS?

[davinci26]

Should it be named windows-something, or are doc comments good enough?

I wanted to keep the property away from the platform in case someone wants to use it for a POSIX concept. Even though I can't think of anything I wanted to keep the api flexible for extensibility in the future.

Is there any facility to update a running envoy if the trust store changes, similar to SDS?

Yeah if you want to use certificates stored in the store and not in the file, I don't think there is a good way to for envoy to know about updates. i.e. I am not sure if its possible with the win32 api to implement watched_directory. Someone would need to "tell" envoy to reload all the certificates.

[davinci26]

Updating the thread:

There is a way to get notifications and callbacks when the store value changes certcontrolstore

[davinci26]

@mattklein123 @ggreenway I am think more of the implementation and I want to make a big change that will probably cascade over various files so I want to get your opinions first.

@wrowe for thoughts

I want to change:

  /**
   * @return full file content as a string.
   * @throw EnvoyException if the file cannot be read.
   * Be aware, this is not most highly performing file reading method.
   */
  virtual std::string fileReadToEnd(const std::string& path) PURE;

to:

  /**
   * @return full file content as a string.
   * @throw EnvoyException if the file cannot be read.
   * Be aware, this is not most highly performing file reading method.
   */
  virtual std::string ReadToEnd(const std::string& path, FileLocationType filelocationtype) PURE;

with:

enum FileLocationType {
    File,
    #ifdef WIN32
    CertificateStore
    #endif
}

The reason I am consider this approach is the following:

• It fits better the unix model of everything being a file. Intuitively I think this will give us a good extension point for windows issues without polluting the API with implementation specific code.
• It can be used as more things get added to Config::DataSource::read

If you think this is a good idea, I can make a separate PR for this change

[mattklein123]

@davinci26 I guess I'm wondering what's the difference between putting the ifdef in the enum vs. just using an ifdef to call a different function? How many callsites are we talking about? Is the issue that you want to keep the calling code generic? How will you handle someone configuring cert store on windows if cert is behind an ifdef?

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[davinci26]

I won't have time to deal with this PR and the priority of this change has dropped. Closing and reopening after 1.18 release