T5661: Add show show ssh dynamic-protection attacker and show log ssh… #2369

JeffWDH · 2023-10-16T20:03:34Z

… dynamic-protection

Change Summary

Adds the following:
show log ssh dynamic-protection
show ssh dynamic-protection

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, renaming)
Refactoring (no functional changes)
Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
Other (please describe):

Related Task(s)

https://vyos.dev/T5661

Related PR(s)

Component(s) name

show log ssh dynamic-protection
show ssh dynamic-protection

Proposed changes

show log ssh dynamic-protection
show ssh dynamic-protection

How to test

admin@vyos:~$ show ssh dynamic-protection
No blocked attackers.

##fail SSH attempts##

admin@vyos:~$ show ssh dynamic-protection
Blocked attackers:
x.x.x.x
admin@vyos:~$

admin@vyos:~$ show log ssh dynamic-protection |tail -n 1
Oct 16 20:00:56 sshguard[426224]: Blocking "x.x.x.x/32" for 3840 secs (3 attacks in 11 secs, after 6 abuses over 15553 secs.)

Smoketest result

Checklist:

I have read the CONTRIBUTING document
I have linked this PR to one or more Phabricator Task(s)
I have run the components SMOKETESTS if applicable
My commit headlines contain a valid Task id
My change requires a change to the documentation
I have updated the documentation accordingly

JeffWDH · 2023-10-16T20:35:46Z

Confusingly, show-ssh-fingerprints.py shows up in this PR - because I made it executable I guess.

$ ls -l show-ssh-*
-rwxr-xr-x 1 jeff jeff 1341 Oct 16 16:25 show-ssh-dynamic-protection-attackers.py
-rwxr-xr-x 1 jeff jeff 1714 Oct 16 16:25 show-ssh-fingerprints.py

op-mode-definitions/show-ssh.xml.in

c-po · 2023-10-17T05:02:20Z

Confusingly, show-ssh-fingerprints.py shows up in this PR - because I made it executable I guess.
$ ls -l show-ssh-*
-rwxr-xr-x 1 jeff jeff 1341 Oct 16 16:25 show-ssh-dynamic-protection-attackers.py
-rwxr-xr-x 1 jeff jeff 1714 Oct 16 16:25 show-ssh-fingerprints.py

Permission changes are also recorded in Git, that's why the file pops up. Please remove this change from this PR.

sever-sever · 2023-10-17T19:28:31Z

show-ssh-dynamic-protection is not good name for new op-mode module
Especially how it will looks like for Graphql or raw request + show_attackers function

I’d request for review from @jestabro and @dmbaturin before merging

I want to see more clear and short names, especially if we want to use it for graphql

there are examples of “good names” https://github.com/vyos/vyos-1x/blob/current/data/op-mode-standardized.json

JeffWDH · 2023-10-17T19:53:30Z

So it sounds like the "right way" would be to combine the functions of show-ssh-dynamic-protection and show-ssh-fingerprints into a single ssh.py?

c-po · 2023-10-17T20:07:49Z

So it sounds like the "right way" would be to combine the functions of show-ssh-dynamic-protection and show-ssh-fingerprints into a single ssh.py?

That would be the best way indeed.

JeffWDH · 2023-10-17T21:11:15Z

Seeing as it's so fresh in my memory, I can combine the two into a single ssh.py. I should have something ready sometime tomorrow.

JeffWDH · 2023-10-18T12:33:20Z

This has been modified to include both the show ssh dynamic-protection and show ssh fingerprints commands in ssh.py, while using the new op mode style. Supports human readable and --raw output for all your API needs.

sever-sever · 2023-10-19T19:29:48Z

src/op_mode/ssh.py

+            pass
+        if attackers:
+            if raw:
+                return {"response": attackers}


As I understand we don’t need extra “response” here
@jestabro what do you think?

I have removed the "response" from the raw sections (except the errors)

dmbaturin

I like the idea but the implementation needs work.

dmbaturin · 2023-10-21T13:55:13Z

src/op_mode/ssh.py

+
+def show_fingerprints(raw: bool, ascii: bool):
+    config = ConfigTreeQuery()
+    if not config.exists("service ssh"):


There's a dedicated exception for this case.

dmbaturin · 2023-10-21T13:56:47Z

src/op_mode/ssh.py

+            else:
+                response = "SSH server public key fingerprints:\n"
+                for key in keys:
+                    response = response + key + "\n"


Why not use "\n".join()?

dmbaturin · 2023-10-21T13:57:32Z

src/op_mode/ssh.py

+
+def show_dynamic_protection(raw: bool):
+    config = ConfigTreeQuery()
+    if not config.exists("service ssh dynamic-protection"):


See above, we have an exception specially for this case.

dmbaturin · 2023-10-21T13:58:09Z

src/op_mode/ssh.py

+            else:
+                response = "Blocked attackers:\n"
+                for attacker in attackers:
+                    response = response + attacker + "\n"


Same, I see no reason not to use "\n".join().

dmbaturin · 2023-10-21T14:00:34Z

src/op_mode/ssh.py

+                return response
+        else:
+            if raw:
+                return {"response": "No blocked attackers."}


Imagine how the client will handle it. Logically, this call returns a list of blocked addresses. The client will want to pass that list to a template or similar. With your approach, the client will have to handle two cases: when it's a list as expected, and when it's a custom response with an error message.

Having no blocked attackers is not even an error state — there can be none because no one is attacking.

The API should just return an empty list in this case.

dmbaturin · 2023-10-21T14:11:16Z

src/op_mode/ssh.py

+            for keyfile in publickeys:
+                if ascii:
+                    try:
+                        keys.append(cmd("ssh-keygen -l -v -E sha256 -f " + keyfile))


While ssh-keygen doesn't support any machine-readable output (sadly), I believe we can and should make an attempt to return useful ouput in the raw case.

For example, suppose we have this:

256 SHA256:SCthwOukM2sh24XSgVbz8J+9i1DtEIV4czP7ECVHjtA no comment (ECDSA) +---[ECDSA 256]---+ | .. ..ooo+ | | .= . +oE= | | ...B o.o.=. | |..+. = ooo | |.= o. +oS.o | |*.+ ...oo. . | |.B.. . .. | |o.. . .. | |. . .. | +----[SHA256]-----+

For a user calling this from a script, something like this would be so much easier to work with:

{ "key_size: 256, "type": "ECDSA", "comment": "no comment", "fingerprint": "SCthwOukM2sh24XSgVbz8J+9i1DtEIV4czP7ECVHjtA", "ascii_art": ... }

(Since ssh-keygen says "no comment" when comment is not set and doesn't signal the absense of a comment, I think we should just always include what it says than try to infer if it's not a key with comment explicitly set to "no comment" somehow).

I've made the requested changes. In the process I also "tabulated" the output.

SSH server public key fingerprints: Type Key Size Fingerprint Comment ------- ---------- -------------------------------------------------- ----------- RSA 3072 SHA256:QUsk927nC0ds8AF/CvxTUtBvNDcyEJjoHGQOE+yFZ8U root@debian DSA 1024 SHA256:fWZsMLLdPOJ8RoO+Qo8ThizDUlIoDWkhhTaBK28Nlcs root@vyos ED25519 256 SHA256:NGYZ85iN4dbzQVWPrzzxpvxlQLo5sndUsjLOYGYISHU root@debian ECDSA 256 SHA256:TsFZT0f4TvlJjKjiIcCRTKPOf459v3oXA0IZiPe76jI root@debian show_fingerprints --raw: [ { "type": "RSA", "key_size": "3072", "fingerprint": "SHA256:QUsk927nC0ds8AF/CvxTUtBvNDcyEJjoHGQOE+yFZ8U", "comment": "root@debian" }, { "type": "DSA", "key_size": "1024", "fingerprint": "SHA256:fWZsMLLdPOJ8RoO+Qo8ThizDUlIoDWkhhTaBK28Nlcs", "comment": "root@vyos" }, { "type": "ED25519", "key_size": "256", "fingerprint": "SHA256:NGYZ85iN4dbzQVWPrzzxpvxlQLo5sndUsjLOYGYISHU", "comment": "root@debian" }, { "type": "ECDSA", "key_size": "256", "fingerprint": "SHA256:TsFZT0f4TvlJjKjiIcCRTKPOf459v3oXA0IZiPe76jI", "comment": "root@debian" } ] SSH server public key fingerprints: Type Key Size Fingerprint Comment ASCII Art ------- ---------- -------------------------------------------------- ----------- ------------------- RSA 3072 SHA256:QUsk927nC0ds8AF/CvxTUtBvNDcyEJjoHGQOE+yFZ8U root@debian +---[RSA 3072]----+ | .+=+O.+o+o. | | o=@ E o +.oo| | . *.+ = + =o+| | . o o * * o| | S o X . | | . = . | | . o | | o . | | . | +----[SHA256]-----+ DSA 1024 SHA256:fWZsMLLdPOJ8RoO+Qo8ThizDUlIoDWkhhTaBK28Nlcs root@vyos +---[DSA 1024]----+ |+O+. . | |*+o + | |oo.+ . . o | |o o E = B | |.. * . .S = % | | + = o ++ B o | | . . o o ++ o | | + .+ | | o. | +----[SHA256]-----+ ... show_fingerprints --ascii --raw [ { "type": "RSA", "key_size": "3072", "fingerprint": "SHA256:QUsk927nC0ds8AF/CvxTUtBvNDcyEJjoHGQOE+yFZ8U", "comment": "root@debian", "ascii_art": "+---[RSA 3072]----+\n| .+=+O.+o+o. |\n| o=@ E o +.oo|\n| . *.+ = + =o+|\n| . o o * * o|\n| S o X . |\n| . = . |\n| . o |\n| o . |\n| . |\n+----[SHA256]-----+" }, { "type": "DSA", "key_size": "1024", "fingerprint": "SHA256:fWZsMLLdPOJ8RoO+Qo8ThizDUlIoDWkhhTaBK28Nlcs", "comment": "root@vyos", "ascii_art": "+---[DSA 1024]----+\n|+O+. . |\n|*+o + |\n|oo.+ . . o |\n|o o E = B |\n|.. * . .S = % |\n| + = o ++ B o |\n| . . o o ++ o |\n| + .+ |\n| o. |\n+----[SHA256]-----+" }, { "type": "ED25519", "key_size": "256", "fingerprint": "SHA256:NGYZ85iN4dbzQVWPrzzxpvxlQLo5sndUsjLOYGYISHU", "comment": "root@debian", "ascii_art": "+--[ED25519 256]--+\n| .. E .....|\n| . o % . ..|\n| . . % = . o .|\n| . .= . o +...|\n| .S. o o+.|\n| . = o+o= |\n| +.++++ =|\n| o+o.=.|\n| .. .o..|\n+----[SHA256]-----+" }, { "type": "ECDSA", "key_size": "256", "fingerprint": "SHA256:TsFZT0f4TvlJjKjiIcCRTKPOf459v3oXA0IZiPe76jI", "comment": "root@debian", "ascii_art": "+---[ECDSA 256]---+\n| ooo ..o . .oo |\n| .=.o + o o.. |\n| .. o o + o.o. |\n|o o o o . .+o |\n| o . S o o...|\n| . . * . o ...|\n| . .o = o |\n| E= + . . |\n| .+=o.+oo |\n+----[SHA256]-----+" } ]

… dynamic-protection

JeffWDH · 2023-10-26T14:44:48Z

When this gets approved, I believe it should be an easy backport to 1.4. A pre-requisite backport would be #2363 however, as it contains the op command definitions for the 'show ssh fingerprints' command which was improved in this one.

github-actions bot added the current label Oct 16, 2023

github-actions bot assigned JeffWDH Oct 16, 2023

vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po and removed request for a team October 16, 2023 20:03

sever-sever reviewed Oct 16, 2023

View reviewed changes

op-mode-definitions/show-ssh.xml.in Outdated Show resolved Hide resolved

c-po requested changes Oct 17, 2023

View reviewed changes

op-mode-definitions/show-ssh.xml.in Outdated Show resolved Hide resolved

JeffWDH requested a review from c-po October 17, 2023 16:09

c-po approved these changes Oct 17, 2023

View reviewed changes

sever-sever approved these changes Oct 18, 2023

View reviewed changes

sever-sever reviewed Oct 19, 2023

View reviewed changes

dmbaturin requested changes Oct 21, 2023

View reviewed changes

T5661: Add show show ssh dynamic-protection attacker and show log ssh…

8cdf6eb

… dynamic-protection

JeffWDH requested a review from dmbaturin October 22, 2023 11:55

dmbaturin approved these changes Oct 26, 2023

View reviewed changes

dmbaturin merged commit 0e129df into vyos:current Oct 26, 2023

JeffWDH mentioned this pull request Oct 26, 2023

T5653: Command to display SSH server public key fingerprints #2363

Merged

12 tasks

This was referenced Oct 26, 2023

T5653: Command to display SSH server public key fingerprints (backport #2363) #2409

Closed

T5661: Add show ssh dynamic-protection and show log ssh dynamic-protection #2412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T5661: Add show show ssh dynamic-protection attacker and show log ssh… #2369

T5661: Add show show ssh dynamic-protection attacker and show log ssh… #2369

JeffWDH commented Oct 16, 2023 •

edited

Loading

JeffWDH commented Oct 16, 2023

c-po commented Oct 17, 2023

sever-sever commented Oct 17, 2023 •

edited

Loading

JeffWDH commented Oct 17, 2023

c-po commented Oct 17, 2023

JeffWDH commented Oct 17, 2023

JeffWDH commented Oct 18, 2023

sever-sever Oct 19, 2023

JeffWDH Oct 19, 2023 •

edited

Loading

dmbaturin left a comment

dmbaturin Oct 21, 2023

dmbaturin Oct 21, 2023

dmbaturin Oct 21, 2023

dmbaturin Oct 21, 2023

dmbaturin Oct 21, 2023

dmbaturin Oct 21, 2023

JeffWDH Oct 21, 2023

JeffWDH commented Oct 26, 2023

T5661: Add show show ssh dynamic-protection attacker and show log ssh… #2369

T5661: Add show show ssh dynamic-protection attacker and show log ssh… #2369

Conversation

JeffWDH commented Oct 16, 2023 • edited Loading

Change Summary

Types of changes

Related Task(s)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

Checklist:

JeffWDH commented Oct 16, 2023

c-po commented Oct 17, 2023

sever-sever commented Oct 17, 2023 • edited Loading

JeffWDH commented Oct 17, 2023

c-po commented Oct 17, 2023

JeffWDH commented Oct 17, 2023

JeffWDH commented Oct 18, 2023

Choose a reason for hiding this comment

JeffWDH Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

dmbaturin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffWDH commented Oct 26, 2023

JeffWDH commented Oct 16, 2023 •

edited

Loading

sever-sever commented Oct 17, 2023 •

edited

Loading

JeffWDH Oct 19, 2023 •

edited

Loading