Update SPAO extension header #4190
Conversation
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/extn.go line 476 at r1 (raw file):
// ParsePacketAuthenticatorOption parses o as a packet authenticator option. // Performs minimal checks to ensure that SPI, Algorithm, timestamp, RSV, and
"algorithm" instead of "Algorithm"
pkg/slayers/extn.go line 495 at r1 (raw file):
// Reuses the OptData buffer if it is of sufficient capacity. func (o PacketAuthenticatorOption) Reset(spi uint32, alg PacketAuthAlg, ts uint32, sn uint32, auth []byte) {
Check that ts and sn are both 24-bit values (instead of just ignoring the higher order bits)?
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/extn.go line 523 at r1 (raw file):
// SPI returns a slice of the underlying SPI buffer. // Changes to this slice will be reflected on the wire when // the extension is serialized.
Wrong comment (not a slice). Same for Timestamp and Sequence number. E.g.
// SPI returns the Security Parameter Index stored in the data buffer.
pkg/slayers/extn.go line 524 at r1 (raw file):
// Changes to this slice will be reflected on the wire when // the extension is serialized. func (o PacketAuthenticatorOption) SPI() uint32 {
Parsing the SPI for usage with DRKey would perhaps also fit into this package.
We could add a type PacketAuthSPI uint32 with methods IsDRKey, DrkeyProto etc. and a func MakePacketAuthSPIDrkey(proto, ...) PacketAuthSPI.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @marcfrei and @matzf)
pkg/slayers/extn.go line 476 at r1 (raw file):
Previously, marcfrei (Marc Frei) wrote…
"algorithm" instead of "Algorithm"
Done.
pkg/slayers/extn.go line 495 at r1 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Check that
tsandsnare both 24-bit values (instead of just ignoring the higher order bits)?
Done.
pkg/slayers/extn.go line 523 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Wrong comment (not a slice). Same for Timestamp and Sequence number. E.g.
// SPI returns the Security Parameter Index stored in the data buffer.
Done.
pkg/slayers/extn.go line 524 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Parsing the SPI for usage with DRKey would perhaps also fit into this package.
We could add a
type PacketAuthSPI uint32with methodsIsDRKey,DrkeyProtoetc. and afunc MakePacketAuthSPIDrkey(proto, ...) PacketAuthSPI.
I added the PacketAuthSPI type. Not sure about this ISDRKey, what's your idea to verify it is indeed a DRKey-type SPI?
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @JordiSubira, @marcfrei, and @matzf)
pkg/slayers/extn.go line 495 at r1 (raw file):
Previously, JordiSubira wrote…
Done.
Ah, sorry, I was not specific enough. Since these are really preconditions, I would simply panic instead of returning an error:
f ts > (1 << 24) {
panic("timestamp must not be larger than 2^24")
}
and
if sn > (1 << 24) {
panic("sequence number must not be larger than 2^24")
}
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @JordiSubira, @marcfrei, and @matzf)
pkg/slayers/extn.go line 495 at r1 (raw file):
Previously, marcfrei (Marc Frei) wrote…
Ah, sorry, I was not specific enough. Since these are really preconditions, I would simply panic instead of returning an error:
f ts > (1 << 24) { panic("timestamp must not be larger than 2^24") }and
if sn > (1 << 24) { panic("sequence number must not be larger than 2^24") }
And the correct condition is as follows, right?
if ts > 1<<24 - 1 {
panic("Timestamp must be smaller than 2^24")
}
Same for sn.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @marcfrei and @matzf)
pkg/slayers/extn.go line 495 at r1 (raw file):
Previously, marcfrei (Marc Frei) wrote…
And the correct condition is as follows, right?
if ts > 1<<24 - 1 { panic("Timestamp must be smaller than 2^24") }Same for
sn.
Good catch! Fixed.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)
pkg/slayers/extn.go line 524 at r1 (raw file):
Ok. This SPI() method should now return a PacketAuthSPI, right?
what's your idea to verify it is indeed a DRKey-type SPI
Yes. The specification of the SPAO allows to non-DRKey use; values >= 2^21 are non-DRKey.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @marcfrei)
pkg/slayers/extn.go line 524 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Ok. This
SPI()method should now return aPacketAuthSPI, right?what's your idea to verify it is indeed a DRKey-type SPI
Yes. The specification of the SPAO allows to non-DRKey use; values >= 2^21 are non-DRKey.
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/extn.go line 453 at r5 (raw file):
// packet authentication option. DRKey values are in the // range [1, 2^21-1]. type PacketAuthSPI uint32
The following types and constants have pretty generic names within the slayers package. Maybe we should prefix them with, e.g., PacketAuth -- I'm not sure, though.
pkg/slayers/extn.go line 491 at r5 (raw file):
func (p PacketAuthSPI) Epoch() DRKeyEpochType { if p^(1<<14) == 0 {
(1<<15)?
pkg/slayers/extn.go line 506 at r5 (raw file):
func MakePacketAuthSPIDrkey(proto uint16, drkeyType DRKeyType, dir DRKeyDirection, epoch DRKeyEpochType) (PacketAuthSPI, error) {
No need to return an error here, see below
pkg/slayers/extn.go line 508 at r5 (raw file):
dir DRKeyDirection, epoch DRKeyEpochType) (PacketAuthSPI, error) { if proto < 1 { return 0, serrors.New("Invalid proto identifier value")
should panic here instead of returning an error
pkg/slayers/extn.go line 511 at r5 (raw file):
} if drkeyType > 1 { return 0, serrors.New("Invalid DRKeyType value")
should panic here instead of returning an error
pkg/slayers/extn.go line 514 at r5 (raw file):
} if dir > 1 { return 0, serrors.New("Invalid DRKeyDirection value")
should panic here instead of returning an error
pkg/slayers/extn.go line 517 at r5 (raw file):
} if epoch > 1 { return 0, serrors.New("Invalid DRKeyEpochType value")
should panic here instead of returning an error
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @marcfrei)
pkg/slayers/extn.go line 453 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
The following types and constants have pretty generic names within the slayers package. Maybe we should prefix them with, e.g.,
PacketAuth-- I'm not sure, though.
I decided to use PacketAuthenticator prefix, to be consistent to the already define PacketAuthenticatorOption.
pkg/slayers/extn.go line 491 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
(1<<15)?
Done.
pkg/slayers/extn.go line 506 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
No need to return an error here, see below
Done.
pkg/slayers/extn.go line 508 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
should panic here instead of returning an error
Done.
pkg/slayers/extn.go line 511 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
should panic here instead of returning an error
Done.
pkg/slayers/extn.go line 514 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
should panic here instead of returning an error
Done.
pkg/slayers/extn.go line 517 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
should panic here instead of returning an error
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status:
pkg/slayers/extn.go line 453 at r5 (raw file):
Previously, JordiSubira wrote…
I decided to use
PacketAuthenticatorprefix, to be consistent to the already definePacketAuthenticatorOption.
I'm wondering whether we should prefix the constants as well, not only the types. Disadvantage would be that all the names get so long.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @marcfrei)
pkg/slayers/extn.go line 453 at r5 (raw file):
Previously, marcfrei (Marc Frei) wrote…
I'm wondering whether we should prefix the constants as well, not only the types. Disadvantage would be that all the names get so long.
Yeah you're probably right about both things. I decided to change use the prefix PacketAuth for everything that relates to the Packet Authenticator Option. I might make sense to use this shortening so that names do not get that long. Alternatively we could use SPAO prefix although, I personally don't like having e.g. SPAOSPI.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @JordiSubira)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/extn.go line 450 at r8 (raw file):
} // PacketAuthSPI is the identifier for the key used for the
This file is getting too large. Please move all the SPAO related entities to a file pkt_auth.go.
Also please add a comment at the top of the file that this is the implementation of the SPAO and link to the documentation
pkg/slayers/extn.go line 450 at r8 (raw file):
} // PacketAuthSPI is the identifier for the key used for the
Suggestion:
PacketAuthSPI (Security Parameter Index)pkg/slayers/extn.go line 453 at r8 (raw file):
// packet authentication option. DRKey values are in the // range [1, 2^21-1]. type PacketAuthSPI uint32
Move this type to just above the implementation of its methods
pkg/slayers/extn.go line 455 at r8 (raw file):
type PacketAuthSPI uint32 type PacketAuthDRKeyType uint8
I don't think that the types PacketAuth{DRKeyType,DRKeyDirection,DRKeyEpochType} are necessary. Simply define the constants as 0 and 1 and make them type unit8
pkg/slayers/extn.go line 512 at r8 (raw file):
) PacketAuthSPI { if proto < 1 {
This method should not panic. Instead it should return an error.
pkg/slayers/extn.go line 573 at r8 (raw file):
serrors.New("wrong option type", "expected", OptTypeAuthenticator, "actual", o.OptType) } if len(o.OptData) < 12 {
Define this magic constant and describe why it's 12 bytes
Code quote:
12pkg/slayers/extn.go line 582 at r8 (raw file):
// Reset reinitializes the underlying EndToEndOption with the SPAO data. // Reuses the OptData buffer if it is of sufficient capacity. func (o PacketAuthOption) Reset(
Create a type for the PacketAuthOption parameters and pass that instead
Code snippet:
type PacketAuthOptionParams struct {
SPI PacketAuthSPI
Algorithm PacketAuthAlg
Timestamp uint32
SequenceNumber uint32
}pkg/slayers/extn.go line 590 at r8 (raw file):
) { if ts >= (1 << 24) {
Library functions that panic are dangerous. Especially, when they will receive user input that you cannot control. You can either return an error here or then you need to document clearly under what conditions the function will panic.
pkg/slayers/extn.go line 622 at r8 (raw file):
} // SPI returns returns the value set in the homonym field in the extension.
What's the "homonym field"? Call it the security parameter index
Suggestion:
security parameter indexpkg/slayers/extn_test.go line 546 at r8 (raw file):
} var spi = slayers.MakePacketAuthSPIDRKey(
Make this a single var block
var (
...
)
JordiSubira
force-pushed
the
update_spao
branch
from
2c0d915 to
f91e9ec
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r9, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @shitz)
pkg/slayers/extn.go line 450 at r8 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This file is getting too large. Please move all the SPAO related entities to a file
pkt_auth.go.Also please add a comment at the top of the file that this is the implementation of the SPAO and link to the documentation
Done.
pkg/slayers/extn.go line 450 at r8 (raw file):
} // PacketAuthSPI is the identifier for the key used for the
Done.
pkg/slayers/extn.go line 453 at r8 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Move this type to just above the implementation of its methods
Done.
pkg/slayers/extn.go line 455 at r8 (raw file):
Previously, shitz (Samuel Hitz) wrote…
I don't think that the types
PacketAuth{DRKeyType,DRKeyDirection,DRKeyEpochType}are necessary. Simply define the constants as 0 and 1 and make them typeunit8
Done.
pkg/slayers/extn.go line 512 at r8 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This method should not panic. Instead it should return an error.
Done.
pkg/slayers/extn.go line 573 at r8 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Define this magic constant and describe why it's 12 bytes
Done.
pkg/slayers/extn.go line 582 at r8 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Create a type for the PacketAuthOption parameters and pass that instead
Done.
pkg/slayers/extn.go line 590 at r8 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Library functions that panic are dangerous. Especially, when they will receive user input that you cannot control. You can either return an error here or then you need to document clearly under what conditions the function will panic.
Done.
pkg/slayers/extn.go line 622 at r8 (raw file):
Previously, shitz (Samuel Hitz) wrote…
What's the "homonym field"? Call it the security parameter index
Done.
pkg/slayers/extn_test.go line 546 at r8 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Make this a single var block
var ( ... )
Done.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/pkt_auth.go line 42 at r9 (raw file):
// MinOptDataLen is the minimum size of the SPAO OptData const MinOptDataLen = 12
Suggestion:
MinPacketAuthDataLenpkg/slayers/pkt_auth.go line 172 at r9 (raw file):
o.OptType = OptTypeAuthenticator n := 12 + len(p.Auth) // 4 + 1 + 3 + 1 + 3 + len(data)
This should use the constant.
Suggestion:
MinPacketAuthDataLenpkg/slayers/pkt_auth.go line 172 at r9 (raw file):
o.OptType = OptTypeAuthenticator n := 12 + len(p.Auth) // 4 + 1 + 3 + 1 + 3 + len(data)
This should be added to the docstring of the minlen constant. Ideally, with an indication what each number refers to.
Code quote:
4 + 1 + 3 + 1 + 3pkg/slayers/pkt_auth_test.go line 34 at r9 (raw file):
optAuthMAC = []byte("16byte_mac_foooo") )
This should be included in pkt_auth.go
Code quote:
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
// | NextHdr=UDP | ExtLen | OptType=2 | OptDataLen |
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
// | Security Parameter Index |
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
// | Algorithm | Timestamp |
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
// | RSV | Sequence Number |
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
// | |
// + +
// | |
// + 16-octet MAC data +
// | |
// + +
// | |
// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @JordiSubira and @shitz)
pkg/slayers/pkt_auth.go line 42 at r9 (raw file):
// MinOptDataLen is the minimum size of the SPAO OptData const MinOptDataLen = 12
Done.
pkg/slayers/pkt_auth.go line 172 at r9 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This should use the constant.
Done.
pkg/slayers/pkt_auth.go line 172 at r9 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This should be added to the docstring of the minlen constant. Ideally, with an indication what each number refers to.
Done.
pkg/slayers/pkt_auth_test.go line 34 at r9 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This should be included in
pkt_auth.go
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status:
This PR updates the SCION Packet Authenticator Option (SPAO) to be compliant with the current specification (https://docs.scion.net/en/latest/protocols/authenticator-option.html).
This PR updates the SCION Packet Authenticator Option (a.k.a SPAO) to be compliant with the current specification (https://scion.docs.anapaya.net/en/latest/protocols/authenticator-option.html).
This change is