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

Issue 14: Wildcard hosts #19

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

userwiths
Copy link
Contributor

@userwiths userwiths commented Feb 14, 2024

Issue: #14
Currently this PR is marked as draft, because I'd love to clear out the TODO at the bottom and probably address requests from reviews if there are such.

Still I've tested this locally and at the moment it suits my (rather basic) needs well.

Here is a quick outline of the made changes:

Changes

  • Removed hardcoded default port, because of it, there was no way to tell if the Port value is read from config or if we are using the hardcoded one.
  • Instead, before returning the sshConfigs array, we cycle through it and in case the Port is 0 (the default one) we set it to 22.
  • We keep a list of wildcard hosts in wildcardHosts variable. Where we were adding the host to the config, now we do additional check to see if it is an actual host or a wildcard one. If it is wildcard it goes in to the new variable.
  • After we have parsed/read the config, we check if we have any wildcardHosts added. If we have we call applyWildcardRules with the hosts and wildcard hosts.
    • Note: This call can return an error on theory, I tried to handle them all but still.
  • We try to match the Host values using regex in the form of ^wildcard$ where wildcard is the Host value with * replaced with .*.
  • If we get a match we apply the wildcard by calling mergeSSHConfigs.
    • The function returns nil on success and Error otherwise.
    • It uses reflection to go through the fields of the structure, because otherwise we would have to do another BIG block of if/else conditions and I try to not do that.
    • We apply the value from the wildcard ONLY if there is no other value in place atm (thats the reason the default port was moved after this operation).
  • There four tests TestMultiWildcard, TestUnmatchedWildcardBetween, TestUnmatchedWildcardPrefix, TestUnmatchedWildcardPost. Each of them tests a different placement of the wildcard and one of them tests multiple wildcards.

TODO - Already addressed in latest commit.

  • LocalForwards, RemoteForwards, DynamicForwards - Currently we do not handle those. Might need to implement them specifically and not use reflection, must check. - ✔️
  • Ciphers, MACs - First attempt at using reflection to add to a string array failed. Either read/try a bit more to make it work or do not use reflection, must check. - ✔️
  • At the moment wildcard hosts should work on single file level. If you have a wildcard host inside an Includeed file, it will be applied only to hosts inside this include. - ✔️

* Add test for prefixed and trailing wildcard, and ensure both cases are handled correctly.
* Accidentally covered a few uncovered lines in `lexer.go` with the new tests.
* Added logic to handle MACs, Ciphers and Forwards.
* Added tests for the mentioned additions.
* Currently we are **only** appending to the fields of the basic host config. That means that it gets its own values *and* those of the wildcard host.
…d host handling in `Includes`

I thought about the best way to do it, and came up with the decision to **not** duplicate code.
This required the breakdown of the `parse` function, and the creation of a function that would handle **ONLY** the extraction of host data.
Without default values and such. It has a boolean parameter to return either only the virtual or actual hosts.
@userwiths
Copy link
Contributor Author

So, the last commit does a big change in my opinion, and I will try to explain my train of thoughts.

At the beginning I was using two variables to save Virtual and Actual hosts in the parse (main) function. It calls itself recursively when it has to parse an Include statement (through Parse which calls parse). The issue that comes here is that if parse returns info/records of the virtual host we would end up in the situation described in the attached issue. The virtual host would be displayed and treated as an actual host.

  1. One possibility was changing the signature of the function, this is a no-no in my opinion as it would be a breaking change for people using the module, so I wont comment on it any more and move forward.
  2. We could create another function that could extract only virtual and we would handle them later. But that would mean the whole for + switch/case code to be duplicated. I believe this is not needed.
  3. Because actual and virtual hosts are populated/created the same way, I went with the decision to take-out the part of the parser that read the Host data and add an argument (boolean) to instruct it to return either virtual or actual hosts.

So using option 3, I separated a large chunk of the code in a new function. Tests pass, coverage did not drop and the virtual hosts seem to be working correctly now.

Of course I understand this might not be so easy to review but I'm willing to assist any way I can.

@userwiths userwiths marked this pull request as ready for review February 16, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant