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

Adds Windows security documentation #1821

Merged
merged 6 commits into from
Nov 8, 2022
Merged

Conversation

kolchfa-aws
Copy link
Collaborator

Fixes #1714

Checklist

  • [ x] By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
@kolchfa-aws kolchfa-aws requested a review from a team as a code owner November 5, 2022 21:27
@kolchfa-aws kolchfa-aws self-assigned this Nov 5, 2022
@kolchfa-aws kolchfa-aws added 3 - Tech review PR: Tech review in progress v2.4.0 'Issues and PRs related to version v2.4.0' labels Nov 5, 2022
@kolchfa-aws
Copy link
Collaborator Author

Could I have @peternied or anyone from @opensearch-project/security review this PR please?

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

@kolchfa-aws Looks good! Can you update from securityadmin.sh to securityadmin.bat in the windows section? I will approve once the PR is updated.


On Windows, the equivalent of `securityadmin.sh` is the `securityadmin.bat` script located in the `path/to/opensearch-{{site.opensearch_version}}/plugins/opensearch-security/tools/` directory.

When running the example commands in the preceding sections, use the **command prompt**. You can open the command prompt by entering `cmd` in the search box next to **Start** on the taskbar.
Copy link
Member

Choose a reason for hiding this comment

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

Powershell should work as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Powershell has a different method of calling .bat files. I didn't want to make it too complicated, so I assume the users who are familiar with Powershell can translate the commands easily. Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't assume that. I imagine there are quite a number of people today who are learning how to interact with a terminal for the first time via Powershell.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. I am referencing a thread here https://stackoverflow.com/questions/20645326/safest-way-to-run-bat-file-from-powershell-script. Which method of running the script from Powershell do you think I should include?

For example, to load your initial configuration (all YAML files), use the following command:

```bat
call ./securityadmin.sh -cd ../../../config/opensearch-security/ -icl -nhnv ^
Copy link
Member

Choose a reason for hiding this comment

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

.bat

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you, good catch! I will update.


When running the example commands in the preceding sections, use the **command prompt**. You can open the command prompt by entering `cmd` in the search box next to **Start** on the taskbar.

Note that on Windows you need the `call` command to call a batch script.
Copy link
Member

Choose a reason for hiding this comment

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

Is call necessary? From what I am reading it looks like call was introduced to call batch scripts within a batch script.

Reference: https://stackoverflow.com/a/14732373

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried without call and it didn't work: it said that .\ is not recognized as a command.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Does it work without the .\? i.e. plugins/opensearch-security/tools/securityadmin.bat instead of call ./plugins/opensearch-security/tools/securityadmin.bat

The official documentation from MS seems to indicate that call does not have any effect outside of a script or batch file: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/call

Note

Call has no effect at the command prompt when it is used outside of a script or batch file.

I bring this up because if we can use the same set of commands on both cmd and Powershell then it simplifies the documentation.

Copy link
Member

@cwperks cwperks Nov 7, 2022

Choose a reason for hiding this comment

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

I just tried without call on cmd on Windows Server 2016 Datacenter and it ran the script.

Running: .\plugins\opensearch-security\tools\install_demo_configuration.bat within the OpenSearch home directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cwperks thank you for testing and confirming! It must be me then. I updated the docs.

Copy link
Member

Choose a reason for hiding this comment

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

Last thing to make sure the same command works on both cmd and powershell. In my testing it looks like cmd requires backslashes while powershell can work with either. I think we should make all of these windows commands use backslashes so that it works in both terminals.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting. I'm on Windows 10, and both forward slashes and backslashes work in command prompt. In any case, I have updated the docs to have backslashes only. Thank you!

Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
@kolchfa-aws
Copy link
Collaborator Author

@cwperks Thank you for reviewing! I updated to .bat and addressed the comments - please let me know your thoughts.

Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
Copy link
Contributor

@cwillum cwillum left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@vagimeli vagimeli left a comment

Choose a reason for hiding this comment

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

One suggestion. LGTM

@@ -13,11 +13,11 @@ redirect_from:
The plugin includes demo certificates so that you can get up and running quickly, but before using OpenSearch in a production environment, you must configure it manually:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend breaking this sentence into two sentences: "The plugin includes demo certificates so that you can get up and running quickly. Before using OpenSearch in a production environment, you must configure it manually:"

Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
Copy link
Collaborator

@natebower natebower left a comment

Choose a reason for hiding this comment

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

@kolchfa-aws Please see my comments and change and let me know if you have any questions. Thanks!

parent: Configuration
nav_order: 20
---

# Apply changes using securityadmin.sh
# Apply changes using the securityadmin script
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the heading and the title match (with vs. using)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this situation, it makes perfect sense, so I'll change it. Sometimes though we have to keep them different.


On Windows, the equivalent of `securityadmin.sh` is the `securityadmin.bat` script located in the `\path\to\opensearch-{{site.opensearch_version}}\plugins\opensearch-security\tools\` directory.

When running the example commands in the preceding sections, use the **command prompt** or **Powershell**. Open the command prompt by entering `cmd` or Powershell by entering `powershell` in the search box next to **Start** on the taskbar.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
When running the example commands in the preceding sections, use the **command prompt** or **Powershell**. Open the command prompt by entering `cmd` or Powershell by entering `powershell` in the search box next to **Start** on the taskbar.
When running the example commands in the preceding sections, use the **command prompt** or **Powershell**. Open the command prompt by entering `cmd`, or Powershell by entering `powershell`, in the search box next to **Start** on the taskbar.

@@ -91,7 +91,7 @@ If your node certificates have an Object ID (OID) identifier in the SAN section,

## Configure admin certificates

Admin certificates are regular client certificates that have elevated rights to perform administrative tasks. You need an admin certificate to change the the security plugin configuration using `plugins/opensearch-security/tools/securityadmin.sh` or the REST API. Admin certificates are configured in `opensearch.yml` by stating their DN(s):
Admin certificates are regular client certificates that have elevated rights to perform administrative tasks. You need an admin certificate to change the the security plugin configuration using [`plugins/opensearch-security/tools/securityadmin.sh`]({{site.url}}{{site.baseurl}}/security-plugin/configuration/security-admin/) or the REST API. Admin certificates are configured in `opensearch.yml` by stating their DN(s):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Admin certificates are regular client certificates that have elevated rights to perform administrative tasks. You need an admin certificate to change the the security plugin configuration using [`plugins/opensearch-security/tools/securityadmin.sh`]({{site.url}}{{site.baseurl}}/security-plugin/configuration/security-admin/) or the REST API. Admin certificates are configured in `opensearch.yml` by stating their DN(s):
Admin certificates are regular client certificates that have elevated rights to perform administrative tasks. You need an admin certificate to change the security plugin configuration using [`plugins/opensearch-security/tools/securityadmin.sh`]({{site.url}}{{site.baseurl}}/security-plugin/configuration/security-admin/) or the REST API. Admin certificates are configured in `opensearch.yml` by stating their DN(s):

@@ -7,7 +7,7 @@ nav_order: 4

# YAML files

Before running `securityadmin.sh` to load the settings into the `.opendistro_security` index, configure the YAML files in `config/opensearch-security`. You might want to back up these files so that you can reuse them on other clusters.
Before running [`securityadmin.sh`]({{site.url}}{{site.baseurl}}/security-plugin/configuration/security-admin/) to load the settings into the `.opendistro_security` index, configure the YAML files in `config/opensearch-security`. You might want to back up these files so that you can reuse them on other clusters.
Copy link
Collaborator

Choose a reason for hiding this comment

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

In "on other clusters", is "on" the right preposition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so. It's like "on the computer" or "on the server".

Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
@kolchfa-aws kolchfa-aws merged commit 12715e3 into main Nov 8, 2022
@Naarcha-AWS Naarcha-AWS deleted the Fix1714-windows-security branch December 13, 2022 19:55
@kolchfa-aws kolchfa-aws added the backport 1.3 PR: Backport label for v1.3.x label Dec 13, 2022
@opensearch-trigger-bot
Copy link

The backport to 1.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport/backport-1821-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 12715e30fb0eafb930efcb31ea3db73271eab3e8
# Push it to GitHub
git push --set-upstream origin backport/backport-1821-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3

Then, create a pull request where the base branch is 1.3 and the compare/head branch is backport/backport-1821-to-1.3.

@kolchfa-aws kolchfa-aws added backport 1.3 PR: Backport label for v1.3.x and removed backport 1.3 PR: Backport label for v1.3.x labels Dec 13, 2022
@opensearch-trigger-bot
Copy link

The backport to 1.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport/backport-1821-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 12715e30fb0eafb930efcb31ea3db73271eab3e8
# Push it to GitHub
git push --set-upstream origin backport/backport-1821-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3

Then, create a pull request where the base branch is 1.3 and the compare/head branch is backport/backport-1821-to-1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Tech review PR: Tech review in progress backport 1.3 PR: Backport label for v1.3.x v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Windows] Security Documentation
5 participants