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

bgpd: Split rpki cache command into separate per SSH/TCP #16023

Merged
merged 2 commits into from
May 28, 2024

Conversation

ton31337
Copy link
Member

Closes #16013

Copy link
Contributor

@louis-6wind louis-6wind left a comment

Choose a reason for hiding this comment

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

Not directly related to the fix but SSH server public key is actually known host path
server_pubkey_XX variable naming is incorrect

doc/user/rpki.rst Outdated Show resolved Hide resolved
doc/user/rpki.rst Outdated Show resolved Hide resolved
bgpd/bgp_rpki.c Outdated Show resolved Hide resolved
@ton31337
Copy link
Member Author

Would you like me changing the naming only?

@louis-6wind
Copy link
Contributor

Would you like me changing the naming only?

Yes if you don't mind. The code is correct

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

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

looks good, waiting on other comments

Copy link
Contributor

@louis-6wind louis-6wind left a comment

Choose a reason for hiding this comment

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

LGTM
Thank you for the renaming

Current command (bundled two into one) is absolutely wrong.

When you configure TCP session with the source, the command thinks, that
it's a SSH session with a username.

It's much better to split this into two separate commands where it's much
easier to do the changes in the future (if more options comes in).

Yes, this is a breaking change, but there is no other proper way to overcome
this.

Bonus note how it looks, which also can lead to crashes (due to port 0x0):

```
(gdb) p *cache->tr_config.ssh_config
$11 = {host = 0x5555562f9cd0 "1.1.1.1", port = 0, bindaddr = 0x0,
  username = 0x55555629ad00 "",
  server_hostkey_path = 0x7ffff53667a0 <rpki_create_socket> "Uf\017\357\300H\211\345AWAVAUATSH\201", <incomplete sequence \354\230>, client_privkey_path = 0x0,
  data = 0x0, new_socket = 0x51, connect_timeout = 4143762592,
  password = 0x7ffff6fccca0 <main_arena+96> "\300\"0VUU"}
(gdb) p *cache->tr_config.tcp_config
$12 = {host = 0x5555562f9cd0 "1.1.1.1", port = 0x0, bindaddr = 0x0,
  data = 0x55555629ad00, new_socket = 0x7ffff53667a0 <rpki_create_socket>,
  connect_timeout = 0}
```

Signed-off-by: Donatas Abraitis <[email protected]>
SERVER_PUBKEY is not the best name to describe what it really is.

Signed-off-by: Donatas Abraitis <[email protected]>
@riw777 riw777 merged commit ffaddf3 into FRRouting:master May 28, 2024
9 checks passed
@ton31337 ton31337 deleted the fix/rpki_show_stuff branch May 28, 2024 14:46
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.

rpki: Source address not added to rpki cache server
3 participants