-
Notifications
You must be signed in to change notification settings - Fork 98
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
OIDC auth, custom configs #63
Conversation
Signed-off-by: Sergey Shubin <serg@ssid.name>
Signed-off-by: Sergey Shubin <serg@ssid.name>
…and set them /bin/false shell Signed-off-by: Sergey Shubin <serg@ssid.name>
1. Added the ability to log in via OpenID 2. Added the ability to install custom configuration files for the cluster 3. Added the ability to reconfigure the cluster (in particular, update certificates) when expanding it 4. Added the ability not to change certificates if the cluster composition has not changed, but only the settings have changed. Signed-off-by: Sergey Shubin <serg@ssid.name>
@saravanan30erd @prudhvigodithi please take a look when you have time. |
@ssi444 Thanks for the great work. Since it have lot of changes, I suggest to have separate PR for each of these features which will help us to review and test it in easier way. Becoz we are supporting many linux distro so we might need to test this against most of them. |
Signed-off-by: Sergey Shubin <serg@ssid.name>
@saravanan30erd The fact is that these changes are logically related and it is better to test them together. For example, OpenID alone makes little sense without custom configuration files. Also, all new settings in the "default" state should lead to exactly the same result when starting the playbook as before |
Calling @saravanan30erd @prudhvigodithi again for the review. |
@saravanan30erd Please let me know if you have bandwidth to review this. |
1 similar comment
Thanks @ssi444, LGTM! |
# - name: Security Plugin configuration | debug | ||
# debug: | ||
# msg: "{{ item }} password: {{ lookup('vars', item + '_password') }}" | ||
# with_items: "{{ custom_users_filtered }}" | ||
# run_once: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ssi444 if this is not required please delete the lines.
Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+
- name: Security Plugin configuration | Set passwords for all users from custom config | ||
shell: > | ||
sed -i '/hash: / s,{{ lookup('vars', item + '_password') }},'$(bash {{ os_sec_plugin_tools_path }}/hash.sh -p {{ lookup('vars', item + '_password') }} | tail -1)',' | ||
{{ os_sec_plugin_conf_path }}/internal_users.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ssi444 can the hash value be passed by user directly instead of this being by ad-hoc shell script, as overtime these sed parsing logic would be hard to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, this is possible. But we need to redo the logic of working with user passwords. This task is done by analogy with the task "- name: Security Plugin configuration | Set the Admin user password". Now passwords are transmitted in clear text and hashed when performing the role. If you transmit hashes, this will lead to the need for additional actions on the part of users (pre-downloading the container from opensearch locally, hashing each password), which is not very convenient, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok with me @peterzhuamazon @saravanan30erd if you are ok with this, I can resolve the conversation.
Thank you
@@ -10,6 +20,7 @@ | |||
run_once: true | |||
register: configuration | |||
become: false | |||
# <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this line and any of similar lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+
environment: | ||
JAVA_HOME: "{{ os_home }}/jdk" | ||
run_once: true | ||
when: configuration.changed or copy_custom_security_configs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssi444 so can you add some details on when this block will be triggered, when a password done by user using admin_password
or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the file README.md in the "Prerequisite" section, I added a description of how to set passwords for custom users (line 92-94)
https://github.com/ssi444/opensearch-project_ansible-playbook/blob/4b4bf51a01ac2e6db9a42c78e18914dab695063d/README.md?plain=1#L92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comes for copy_custom_security_configs I guess, just wanted to know about configuration.changed, when an admin user changed ?
@@ -0,0 +1,287 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssi444 having the file directly added might not work if backported to 1.x branch, as config file are differnt for 1.x and 2.x security plugin https://github.com/opensearch-project/security/tree/main/config can you add some details here? If they are different then the backpot PR for 1.x should have a different file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contents of the security_plugin_config.yml file are taken from OpenSearch v.1.3.2. At the time of writing the patch, version 2.0 has not yet been released. If you compare this file with the file https://github.com/opensearch-project/security/blob/main/config/config.yml then there is no change at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @ssi444, ok with me, just added a thought as we might not know if i can change for next major versions.
@peterzhuamazon @saravanan30erd
@@ -34,3 +34,65 @@ cluster_type: multi-node | |||
os_user: opensearch | |||
|
|||
os_dashboards_user: opensearch-dashboards | |||
|
|||
# Number of days that certificates are valid | |||
cert_valid_days: 730 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for 730? Can we have an empty space if null default as 1yr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value was before my edits, so I left it the same. Probably this period was optimal. I have set 10 years for myself, because I do not see the need for frequent certificate changes in the cluster.
with_items: "{{ cert_stat_result.results }}" | ||
when: item.stat.exists == False | ||
|
||
- name: Security Plugin configuration | debug 1 - force_update_cert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssi444 what are these debug 1
debug 2
? Can you please add some details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+
|
||
- name: Security Plugin configuration | Filter service keys from the list of users | ||
set_fact: | ||
custom_users_filtered: '{{ custom_users|dict2items|rejectattr("key", "equalto", "_meta")|list|items2dict }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssi444 can you add some details about this logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comment
Signed-off-by: Sergey Shubin <serg@ssid.name>
@prudhvigodithi check please |
Hi @ssi444, Just so you know, I was reviewing the PR alongside @prudhvigodithi. Recently we are a bit busy on our tasks so review will be slow, especially July4 incoming. Thanks. |
LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Update dashboards.yml. Fix owner and permissions for home folder Signed-off-by: Sergey Shubin <serg@ssid.name> * The validity period of certificates is set to a variable Signed-off-by: Sergey Shubin <serg@ssid.name> * change HOME directory for {{ os_user }} and {{ os_dashboards_user }} and set them /bin/false shell Signed-off-by: Sergey Shubin <serg@ssid.name> * auth_type (internal, openid). Custom configs, IaC 1. Added the ability to log in via OpenID 2. Added the ability to install custom configuration files for the cluster 3. Added the ability to reconfigure the cluster (in particular, update certificates) when expanding it 4. Added the ability not to change certificates if the cluster composition has not changed, but only the settings have changed. Signed-off-by: Sergey Shubin <serg@ssid.name> * readme. description for OpenID, IaC, custom configuration files Signed-off-by: Sergey Shubin <serg@ssid.name> * refactoring. see opensearch-project#63 Signed-off-by: Sergey Shubin <serg@ssid.name>
* Update dashboards.yml. Fix owner and permissions for home folder Signed-off-by: Sergey Shubin <serg@ssid.name> * The validity period of certificates is set to a variable Signed-off-by: Sergey Shubin <serg@ssid.name> * change HOME directory for {{ os_user }} and {{ os_dashboards_user }} and set them /bin/false shell Signed-off-by: Sergey Shubin <serg@ssid.name> * auth_type (internal, openid). Custom configs, IaC 1. Added the ability to log in via OpenID 2. Added the ability to install custom configuration files for the cluster 3. Added the ability to reconfigure the cluster (in particular, update certificates) when expanding it 4. Added the ability not to change certificates if the cluster composition has not changed, but only the settings have changed. Signed-off-by: Sergey Shubin <serg@ssid.name> * readme. description for OpenID, IaC, custom configuration files Signed-off-by: Sergey Shubin <serg@ssid.name> * refactoring. see #63 Signed-off-by: Sergey Shubin <serg@ssid.name> Co-authored-by: ssi444 <55582328+ssi444@users.noreply.github.com>
Hi team, Just to let you know, and make cross reference. This PR introduced a few breaking changes on the default deployment. Please see below a list of issues and PRs for the fixes;
Regards |
@ssi444 ^^ Thanks. |
Thanks @rodolfovillordo we will take a look soon. |
* Update dashboards.yml. Fix owner and permissions for home folder Signed-off-by: Sergey Shubin <serg@ssid.name> * The validity period of certificates is set to a variable Signed-off-by: Sergey Shubin <serg@ssid.name> * change HOME directory for {{ os_user }} and {{ os_dashboards_user }} and set them /bin/false shell Signed-off-by: Sergey Shubin <serg@ssid.name> * auth_type (internal, openid). Custom configs, IaC 1. Added the ability to log in via OpenID 2. Added the ability to install custom configuration files for the cluster 3. Added the ability to reconfigure the cluster (in particular, update certificates) when expanding it 4. Added the ability not to change certificates if the cluster composition has not changed, but only the settings have changed. Signed-off-by: Sergey Shubin <serg@ssid.name> * readme. description for OpenID, IaC, custom configuration files Signed-off-by: Sergey Shubin <serg@ssid.name> * refactoring. see opensearch-project/ansible-playbook#63 Signed-off-by: Sergey Shubin <serg@ssid.name>
Description
Issues Resolved
#61
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.