ripd/ripd.c - rip_auth_md5 : Change the start value of sequence 1 to 0 #16233
Conversation
| @@ -1051,7 +1051,7 @@ static size_t rip_auth_md5_ah_write(struct stream *s, struct rip_interface *ri, | |||
| /* RFC2080: The value used in the sequence number is | |||
| arbitrary, but two suggestions are the time of the | |||
| message's creation or a simple message counter. */ | |||
| stream_putl(s, ++seq); | |||
| stream_putl(s, seq++); | |||
There was a problem hiding this comment.
can you explain to me how what we are doing is wrong or incompatible(the rfc is 2082 actually )? This seems like a bug in bird not frr. The rfc states that the sequence number must increase. It is arbitrary and the two suggestions are a simple counter or the time of the message. If we implemented a time based count how would this also not break bird?
There was a problem hiding this comment.
Hi,
The RFC does not actually define a starting value.
For simplicity's sake, starting from scratch doesn't seem like a bad idea.
Bird expects to have an increasing sequence or zero value when losing connectivity.
It is also possible to go back to the commit for time management
There was a problem hiding this comment.
rfc2082 says:
When connectivity to the
neighbor has been lost, the receiver SHOULD be ready to accept
either:
- a message with a sequence number of zero
- a message with a higher sequence number than the last received
sequence number.
To me is sounds valid starting from zero (0), not 1. Where another side can accept it. (using a time solves a second bullet)
There was a problem hiding this comment.
Additionally, this is something that needs to be addressed in bird not FRR. It's valid to send a 0 value to a neighbor and the receiving entity should accept it..
There was a problem hiding this comment.
While starting from 0 is valid, I think it'd be better to start from time(NULL), and then move incrementally from there. That would be a simple way of solving the problem of the sequence number going backwards after restarting the daemon (#15049).
There was a problem hiding this comment.
Please fix the commit according to our contribution guidelines: https://github.com/FRRouting/frr/blob/master/doc/developer/workflow.rst#submitting-patches-and-enhancements
T-Nicolas
changed the title
|
@frrbot rereview |
|
Please sign the commit (more details: https://github.com/FRRouting/frr/pull/16233/checks?check_run_id=26663033876) |
Signed-off-by: T-Nicolas <[email protected]>
|
@Mergifyio backport dev/10.1 stable/10.0 stable/9.1 stable/9.0 |
✅ Backports have been created
|
ripd/ripd.c - rip_auth_md5 : Change the start value of sequence 1 to 0 (backport #16233)
ripd/ripd.c - rip_auth_md5 : Change the start value of sequence 1 to 0 (backport #16233)
ripd/ripd.c - rip_auth_md5 : Change the start value of sequence 1 to 0 (backport #16233)
ripd/ripd.c - rip_auth_md5 : Change the start value of sequence 1 to 0 (backport #16233)
Start the sequence at zero to make md5 authentication compatible with bird