Skip to content

Conversation

@intekhab1025
Copy link
Contributor

This PR adds comprehensive SSL/TLS support to SaltGUI with automated setup scripts, Docker Compose configuration, and Salt state management for secure deployments.
✨ Features Added
🔐 SSL/TLS Infrastructure
Self-signed certificate generation for development/testing
Docker Compose SSL environment with HTTPS endpoints
Salt state for SSL configuration (saltgui-ssl.sls)
Automated testing script for end-to-end SSL verification
📁 New Files
generate-ssl-certs.sh - SSL certificate generation script
test-ssl-deployment.sh - Automated SSL testing
docker-compose-ssl.yml - SSL-enabled Docker environment
master-ssl - Salt master configuration for SSL
saltgui-ssl.sls - Salt state for SSL deployment

@erwindon
Copy link
Owner

erwindon commented Aug 4, 2025

@intekhab1025
I would indeed be very nice to present a SSL/TLS sample setup.
But as before, I cannot support the SLS to install into any environment. It has too many fixed decisions that need changes for other environments. First of all the fixed username/password, but it applies to other setting also.

My proposal is to simplify the code, roughly like this:

  • Add dockerfiles/dockerfile-saltmaster-ssl; similar to dockerfiles/dockerfile-saltmaster, but with all ssl setup steps. It should roughly contain all the logic from master-ssl and generate-ssl-certs.sh;
  • Update build-and-upload.sh to include the new image;
  • Add docker/docker-compose-ssl.yml. It starts the new saltmaster-ssl and the minions;
  • Provide details in the documentation on how to run this using docker-compose and how to connect to it using a browser.

My further proposal is that I make these changes myself for the following reasons:

@intekhab1025
Copy link
Contributor Author

@intekhab1025 I would indeed be very nice to present a SSL/TLS sample setup. But as before, I cannot support the SLS to install into any environment. It has too many fixed decisions that need changes for other environments. First of all the fixed username/password, but it applies to other setting also.

My proposal is to simplify the code, roughly like this:

  • Add dockerfiles/dockerfile-saltmaster-ssl; similar to dockerfiles/dockerfile-saltmaster, but with all ssl setup steps. It should roughly contain all the logic from master-ssl and generate-ssl-certs.sh;
  • Update build-and-upload.sh to include the new image;
  • Add docker/docker-compose-ssl.yml. It starts the new saltmaster-ssl and the minions;
  • Provide details in the documentation on how to run this using docker-compose and how to connect to it using a browser.

My further proposal is that I make these changes myself for the following reasons:

@erwindon - This makes sense, I went ahead and addressed first part of comments. Also readme has been updated with instruction.

@erwindon erwindon marked this pull request as draft August 6, 2025 02:18
@erwindon
Copy link
Owner

erwindon commented Aug 6, 2025

well done!

I wanted to split salt-master and salt-api in the existing docker-compose set. But I have abandoned that. These 2 really belong together and (too) much compensation is needed when you put them in different containers.
The change is now already much more consistent with the previous work.
And this change is now easy enough for me to understand.
So I'll drop the request to make the changes myself.

The review so far is only based on the text of the PR. I may add more comments/requests when I'm testing this.

@erwindon
Copy link
Owner

erwindon commented Aug 13, 2025

@intekhab1025
I still have 2 technical problems with this:

  • The minions in docker-compose-ssl.yml are connecting to saltmaster-local. but the compose file defines saltmaster-local-ssl. that name may be reduced to saltmaster-local again to get this working again. note that in an earlier comment I suggested to use saltmaster-local-tls, but that specific comment is then voided.
  • when I navigate to https://localhost:3334, there is no valid HTTPS service there, the SSL/TLS part fails. do you have better results?

Please also look into the remaining review comments.

@intekhab1025
Copy link
Contributor Author

Yes will check them out, my plan is to address all by this week

@erwindon erwindon force-pushed the feature/SSL-TLS-implementation branch from 0c93638 to 69b8a69 Compare August 21, 2025 01:25
@erwindon
Copy link
Owner

erwindon commented Aug 24, 2025

@intekhab1025
I've made a few changes on the master branch, some of which overlap with your work.
Can you please rebase your branch?

@erwindon
Copy link
Owner

@intekhab1025
In the mean time, I thought on how this can be easier useable.

The existing docker-compose set is used from shell script runtests.sh. But that one shuts down the compose set after the regression tests have run. So that is not convenient to demonstrate a system, whether using tls or not. Note that that was a problem you did not introduce, but the explicit presence of extra compose set strengthens the problem.

Therefore, I want to add the following 2 files:

file docker/runstd.sh:

#!/bin/sh

set -e

# start a salt master, three salt minions and saltgui to run tests on
# Don't use --detach; travis docker does not understand it
docker-compose --file docker-compose.yml up -d

# wait until user decides
echo "Please use http://localhost:3333 for SaltGUI (Username='salt', Password='salt')"
echo -n "Press ENTER to stop $0"; read dummy

# remove the containers
docker-compose --file docker-compose.yml rm --force --stop

echo "DONE!"

# End

and file docker/runtls.sh:

#!/bin/sh

set -e

# start a salt master, three salt minions and saltgui to run tests on
# Don't use --detach; travis docker does not understand it
docker-compose --file docker-compose-tls.yml up -d

# wait until user decides
echo "Please use https://localhost:3334 for SaltGUI (Username='salt', Password='salt')"
echo -n "Press ENTER to stop $0"; read dummy

# remove the containers
docker-compose --file docker-compose-tls.yml rm --force --stop

echo "DONE!"

# End

@erwindon
Copy link
Owner

Please make sure that you can run SaltGUI with TLS with your supplied docker compose file with at least the following steps:

  • SaltGUI login screen shows on https://localhost:3334
  • Login with salt/salt
  • Go to to page Keys
  • See the 3 minions

@intekhab1025
Copy link
Contributor Author

@intekhab1025 I still have 2 technical problems with this:

  • The minions in docker-compose-ssl.yml are connecting to saltmaster-local. but the compose file defines saltmaster-local-ssl. that name may be reduced to saltmaster-local again to get this working again. note that in an earlier comment I suggested to use saltmaster-local-tls, but that specific comment is then voided.
  • when I navigate to https://localhost:3334, there is no valid HTTPS service there, the SSL/TLS part fails. do you have better results?

Please also look into the remaining review comments.

@erwindon - I have fixed the issue so https://localhost:3334 works now, see the attached snapshot. Also I have updated the build script to make it platform independent.
image

@intekhab1025
Copy link
Contributor Author

@intekhab1025 I've made a few changes on the master branch, some of which overlap with your work. Can you please rebase your branch?

Done

@intekhab1025
Copy link
Contributor Author

@intekhab1025 I've made a few changes on the master branch, some of which overlap with your work. Can you please rebase your branch?

@intekhab1025 intekhab1025 reopened this Aug 27, 2025
@intekhab1025 intekhab1025 marked this pull request as ready for review August 27, 2025 00:17
@intekhab1025
Copy link
Contributor Author

Accidentally, I closed the PR but it should be good to go now @erwindon

@erwindon
Copy link
Owner

Ok, nice work! the docker-compose-set with tls is indeed working nicely.

but I have problems with the change:

  • you have merged the master branch into your development branch.
    this makes it very unclear to see which changes are really part of the new feature, and which changes that were injected in another way.
    development branches must be 'rebased' to update them (no 'merge' from main/master).
  • there are too many changes on the branch that deal with non-related functionality (e.g. the salt-states). yes it is removed again in later commits, but the branch contains the full history of adding it and removing it.
  • most of the changes in the build-and-upload.sh script have a different purpose than the original PR. i.e.: tls vs multi-arch.
    that is not acceptable in this PR. but I can accept a separate PR for multi-arch.
    further, you have updated that script with comments that are not descriptive to the functionality. e.g. describing what is "unchanged".

I foresee that it will take (too) many iterations to get this right.
Therefore, I intend to re-implement this PR by git-squashing your code and leaving the unrelated parts behind.

@intekhab1025
Copy link
Contributor Author

Ok, nice work! the docker-compose-set with tls is indeed working nicely.

but I have problems with the change:

  • you have merged the master branch into your development branch.
    this makes it very unclear to see which changes are really part of the new feature, and which changes that were injected in another way.
    development branches must be 'rebased' to update them (no 'merge' from main/master).
  • there are too many changes on the branch that deal with non-related functionality (e.g. the salt-states). yes it is removed again in later commits, but the branch contains the full history of adding it and removing it.
  • most of the changes in the build-and-upload.sh script have a different purpose than the original PR. i.e.: tls vs multi-arch.
    that is not acceptable in this PR. but I can accept a separate PR for multi-arch.
    further, you have updated that script with comments that are not descriptive to the functionality. e.g. describing what is "unchanged".

I foresee that it will take (too) many iterations to get this right. Therefore, I intend to re-implement this PR by git-squashing your code and leaving the unrelated parts behind.

I see, appreciate your help. Let me know if you would like me to instead clean up that, i can have a look tomorrow.

@intekhab1025 intekhab1025 force-pushed the feature/SSL-TLS-implementation branch 3 times, most recently from 45ea1e0 to 33105c6 Compare August 27, 2025 09:39
@intekhab1025
Copy link
Contributor Author

Ok, nice work! the docker-compose-set with tls is indeed working nicely.

but I have problems with the change:

  • you have merged the master branch into your development branch.
    this makes it very unclear to see which changes are really part of the new feature, and which changes that were injected in another way.
    development branches must be 'rebased' to update them (no 'merge' from main/master).
  • there are too many changes on the branch that deal with non-related functionality (e.g. the salt-states). yes it is removed again in later commits, but the branch contains the full history of adding it and removing it.
  • most of the changes in the build-and-upload.sh script have a different purpose than the original PR. i.e.: tls vs multi-arch.
    that is not acceptable in this PR. but I can accept a separate PR for multi-arch.
    further, you have updated that script with comments that are not descriptive to the functionality. e.g. describing what is "unchanged".

I foresee that it will take (too) many iterations to get this right. Therefore, I intend to re-implement this PR by git-squashing your code and leaving the unrelated parts behind.

@erwindon - I have cleaned up , squashed all commits and rebased. Please check now

@intekhab1025 intekhab1025 requested a review from erwindon August 27, 2025 09:42
@erwindon
Copy link
Owner

OK! almost there, and less work for me. just one more detail:
the indentation in build-and-upload.sh is inconsistent.
please use git amend+forcepush if you still want to fix that (not strictly needed, see below).

note that I have the following followup-changes in mind:

  • update version number to 3007.6
  • stop exposing ports 4505+4506 from docker-compose
    that way, it does not conflict with the regular saltstack installations
  • adding the 2 files runstd.sh and runtls.sh I showed earlier
  • update build-and-upload.sh to stop pushing to docker-hub and which will then be abandoned. and giving the script the new name build.sh or so.

@intekhab1025 intekhab1025 force-pushed the feature/SSL-TLS-implementation branch 3 times, most recently from da23b77 to 7087700 Compare August 27, 2025 16:41
@intekhab1025
Copy link
Contributor Author

OK! almost there, and less work for me. just one more detail: the indentation in build-and-upload.sh is inconsistent. please use git amend+forcepush if you still want to fix that (not strictly needed, see below).

note that I have the following followup-changes in mind:

  • update version number to 3007.6
  • stop exposing ports 4505+4506 from docker-compose
    that way, it does not conflict with the regular saltstack installations
  • adding the 2 files runstd.sh and runtls.sh I showed earlier
  • update build-and-upload.sh to stop pushing to docker-hub and which will then be abandoned. and giving the script the new name build.sh or so.

Okay, good catch, fixed the indentation.
The idea of other changes sounds good. Shouldn't we consider these changes in a different PR to make this PR change consistence?

@intekhab1025 intekhab1025 force-pushed the feature/SSL-TLS-implementation branch from 7087700 to 0b0fd9f Compare August 27, 2025 16:48
This commit adds comprehensive TLS support to SaltGUI with the following features:

- Added TLS-enabled Docker Compose configuration (docker-compose-tls.yml)
- Created saltmaster-tls Docker configuration with SSL certificates
- Updated build script
- Added comprehensive TLS setup documentation in README
- Implemented secure HTTPS endpoint on port 3334
- Restored core files to maintain consistency with master branch

The implementation allows users to run SaltGUI with full TLS encryption
for secure communication between the web interface and Salt master.
@intekhab1025 intekhab1025 force-pushed the feature/SSL-TLS-implementation branch from 0b0fd9f to 15f73ec Compare August 27, 2025 16:50
@sonarqubecloud
Copy link

@erwindon erwindon merged commit 1ed9e59 into erwindon:master Aug 27, 2025
8 checks passed
@erwindon
Copy link
Owner

NOTE: I added the word "sample" to the merge commit

@erwindon
Copy link
Owner

@intekhab1025
thx!

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.

2 participants