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

Add mutual TLS docs #1132

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Add mutual TLS docs #1132

merged 3 commits into from
Jul 8, 2024

Conversation

kilfoyle
Copy link
Contributor

@kilfoyle kilfoyle commented Jun 13, 2024

Thanks for the nice draft @nimarezainia! I made only very, very small changes.

@AndersonQ and @michel-laterman Let me know if you're not the right folks to review this. I'm not sure who worked on the feature.

Please see Docs Preview

There are also some mTLS docs changes in #1099

Closes: #1131

Copy link

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

@kilfoyle kilfoyle marked this pull request as ready for review June 13, 2024 20:44
@kilfoyle kilfoyle requested a review from a team as a code owner June 13, 2024 20:44
@nimarezainia
Copy link
Contributor

@michel-laterman @AndersonQ would you mind reviewing these to ensure accuracy with what we have in code. TIA

Copy link
Member

@AndersonQ AndersonQ left a comment

Choose a reason for hiding this comment

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

Folks, here are my 2 (or 22) cents.

There are a few disparities between what is written and how things actually happen or are. Those I marked with [Blocker], they need to be addressed or [Blocker-ish] I'd rather see them addressed, but if you understand my point and still believe the current version is good, I won't make a stand.

The ones marked with [Suggestion] are exactly that, suggestions, feel free to ignore, no need to explain to me why.

Also I've put some suggested changes, those are just suggestion, feel free to use whatever wording you find more appropriated. I just used it as a tool to make my point clear rather than asking it to b taken as I put.

docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
@kilfoyle
Copy link
Contributor Author

Thanks for the excellent review @AndersonQ! 🙏
I've addressed all of your comments, most of them exactly as suggested.

@kilfoyle kilfoyle requested a review from AndersonQ June 19, 2024 21:05
Copy link
Member

@AndersonQ AndersonQ left a comment

Choose a reason for hiding this comment

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

@kilfoyle my bad here, I should have explained better about the configuration name/key/flag.

Actually I've just realised the design doc was wrong as well, it's fixed now.

When it's a cli parameter or flag, it's in the form --+name-of-the-flag (--name-of-the-flag).

For the policy, it's a YAML path like some.yaml.path.to.my_config_name.

Below are the correct forms

tl;dt

  • in the policy (the one for the advanced YAML box):
ssl.certificate_authorities:
  - /path/to/ca
ssl.certificate: /path/to/cert
ssl.key: /path/to/cert_key

# OR

  ssl:
    certificate_authorities:
      - /path/to/ca
    certificate: /path/to/cert
    key: /path/to/cert_key
  • for the cli (install/enroll cmd)
--certificate-authorities
--elastic-agent-cert
--elastic-agent-cert-key

--certificate-authorities # this is the same cli flag as the one above
--fleet-server-cert
--fleet-server-cert-key

--fleet-server-es-ca
--fleet-server-es-cert
--fleet-server-es-cert-key

I suggested a fix for the 1st set of cli flags and there are 2 other minor comments

docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
docs/en/ingest-management/security/mutual-tls.asciidoc Outdated Show resolved Hide resolved
@kilfoyle
Copy link
Contributor Author

@AndersonQ this is great. I've fixed up all of the settings as you suggested.

For the policy settings that go in the "advanced YAML" box, I updated the screen capture so the example uses the correct format, and I also added the following in that section of the page, so that people will be sure to know what format to use:

Screenshot 2024-06-20 at 11 57 30 AM

@kilfoyle
Copy link
Contributor Author

kilfoyle commented Jul 2, 2024

@AndersonQ When you have some time can you please give this one a final check?

@nimarezainia
Copy link
Contributor

@AndersonQ in context of this issue - could you please provide the updated cli flag description that you envisage so that the docs can reference the ideal description text and whoever takes over the issue can update the helper text according to what we place in the docs.

cc: @pierrehilbert

@pierrehilbert
Copy link
Contributor

@AndersonQ is on SDH rotation this week, unless the week is really noisy from that perspective, he should have time to do the final check soon.

@AndersonQ
Copy link
Member

sorry for the delay, the SDHs were keeping me quite busy, it's approved now.

@pierrehilbert
Copy link
Contributor

Thanks @AndersonQ
@kilfoyle we are now good to go.

@AndersonQ
Copy link
Member

@AndersonQ in context of this issue - could you please provide the updated cli flag description that you envisage so that the docs can reference the ideal description text and whoever takes over the issue can update the helper text according to what we place in the docs.

cc: @pierrehilbert

I updated elastic/elastic-agent#5037 with a proposed changes and opened elastic/elastic-agent#5048

@kilfoyle
Copy link
Contributor Author

kilfoyle commented Jul 3, 2024

Brilliant! Thanks a lot @AndersonQ!

@kilfoyle kilfoyle merged commit d5358d2 into elastic:main Jul 8, 2024
3 checks passed
mergify bot pushed a commit that referenced this pull request Jul 8, 2024
* Add mutual TLS docs

* Address comments

* Add fixes for latest comments

(cherry picked from commit d5358d2)
kilfoyle added a commit that referenced this pull request Jul 8, 2024
* Add mutual TLS docs

* Address comments

* Add fixes for latest comments

(cherry picked from commit d5358d2)

Co-authored-by: David Kilfoyle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document Elastic Agent deployment models with mutual TLS
5 participants