epic-hp: dataplane #3951
epic-hp: dataplane #3951
Conversation
mawyss
force-pushed
the
epic4
branch
2 times, most recently
from
3628256
to
667100f
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 13 files reviewed, 5 unresolved discussions (waiting on @mawyss)
go/lib/libepic/libepic.go, line 15 at r2 (raw file):
// limitations under the License. package libepic
Please consider just epic, otherwise we have too many lib prefixes.
If needed import it as libepic when used.
go/lib/libepic/libepic.go, line 37 at r2 (raw file):
const ( // Error messages ErrCipherFailure common.ErrMsg = "Unable to initialize AES cipher"
Do we need to export them? Also , check how the errors are created in https://github.com/scionproto/scion/blob/master/go/pkg/router/dataplane.go#L105-L115
go/lib/libepic/libepic.go, line 40 at r2 (raw file):
ErrMacFailure common.ErrMsg = "Unable to initialize Mac" // Maximal lifetime of a packet in milliseconds PacketLifetimeMs uint16 = 2000
Do we need to export them?
If you need them for testing, see https://github.com/scionproto/scion/blob/master/go/pkg/router/export_test.go
go/lib/libepic/libepic.go, line 50 at r2 (raw file):
return nil, serrors.Wrap(ErrCipherFailure, err) } // todo: CMAC is not ideal for EPIC due to its subkey generation overhead.
This todo in the code is not very clear, maybe you could create an issue describing the
limitation and how we should change this in the future.
go/lib/libepic/libepic.go, line 92 at r2 (raw file):
} func PrepareMacInput(epicpath *epic.EpicPath, s *slayers.SCION, timestamp uint32) ([]byte, error) {
Please bring the exported functions on the beginning of the file and have for each one a comment according to go standards.
You should be able to review how the comment looks like by godoc -http=:6060
(always check if you can avoid exporting the function, in which case you do not need comment)
General comments can be consolidated into a doc.go file e.g. https://github.com/scionproto/scion/blob/master/go/pkg/gateway/routing/doc.go
There was a problem hiding this comment.
Reviewable status: 0 of 15 files reviewed, 5 unresolved discussions (waiting on @karampok)
go/lib/libepic/libepic.go, line 15 at r2 (raw file):
Previously, karampok (Konstantinos) wrote…
Please consider just
epic, otherwise we have too manylibprefixes.
If needed import it aslibepicwhen used.
Done. Changed libepic to epic.
go/lib/libepic/libepic.go, line 37 at r2 (raw file):
Previously, karampok (Konstantinos) wrote…
Do we need to export them? Also , check how the errors are created in https://github.com/scionproto/scion/blob/master/go/pkg/router/dataplane.go#L105-L115
Done. Created the errors as described in your referenced file (and without exporting them).
go/lib/libepic/libepic.go, line 40 at r2 (raw file):
Previously, karampok (Konstantinos) wrote…
Do we need to export them?
If you need them for testing, see https://github.com/scionproto/scion/blob/master/go/pkg/router/export_test.go
Done. Thank you for the hint!
go/lib/libepic/libepic.go, line 50 at r2 (raw file):
Previously, karampok (Konstantinos) wrote…
This todo in the code is not very clear, maybe you could create an issue describing the
limitation and how we should change this in the future.
Done. Instead of creating an issue I solved it in the meantime.
go/lib/libepic/libepic.go, line 92 at r2 (raw file):
Previously, karampok (Konstantinos) wrote…
Please bring the exported functions on the beginning of the file and have for each one a comment according to go standards.
You should be able to review how the comment looks like bygodoc -http=:6060(always check if you can avoid exporting the function, in which case you do not need comment)
General comments can be consolidated into a doc.go file e.g. https://github.com/scionproto/scion/blob/master/go/pkg/gateway/routing/doc.go
Done. Added comments for the exported functions and reordered them. Some of the exported functions are indeed only used for the tests at the moment, they will be used for the packet generation at the end hosts however (which will be a separate PR).
There was a problem hiding this comment.
Reviewed 2 of 13 files at r4.
Reviewable status: 2 of 15 files reviewed, 12 unresolved discussions (waiting on @karampok and @mawyss)
go/pkg/router/dataplane.go, line 675 at r5 (raw file):
origPacket []byte, buffer gopacket.SerializeBuffer) (processResult, error) { // Get the already parsed EPIC path header
you can also drop the comments, as the code should be clear what it does (all comments within that function)
go/pkg/router/dataplane.go, line 689 at r5 (raw file):
// Parse the current info field to get the timestamp info, err := scionRaw.GetCurrentInfoField() if err != nil || info == nil {
The implementation of a function that returns (X,error) should (ideally) return a nil X only if err is not nil. And a not-nil X if err is nil.
In other words, if the err is nil you expect the X to be meaningful happy path.
As of now, you hide what is happening under the malformedPath error string
go/pkg/router/dataplane.go, line 715 at r5 (raw file):
// Get the cached authenticator auth := p.cachedMac if len(auth) != 16 {
this value should be a constant inside the epic package
go/pkg/router/dataplane.go, line 721 at r5 (raw file):
// Verify the PHVF and LHVF if necessary if b, err := libepic.IsPenultimateHop(scionRaw); b {
This if/else needs to be refactored to avoid else using a switch statement https://golang.org/doc/effective_go.html#switch. It is more go idiomatic and better readable.
Also, I suggest that this if/else is hidden into a unique function that is inside the epic package
go/pkg/router/dataplane.go, line 730 at r5 (raw file):
} if !ok { // todo: send back scmp packet?
There should be no todo. You can remove it and add later a PR about scmp. Also
// TODO(name): is the format we currently use
go/pkg/router/dataplane.go, line 857 at r5 (raw file):
} p.path = epicpath.ScionRaw } else {
please provide an implementation without else
go/pkg/router/dataplane_test.go, line 53 at r5 (raw file):
) var cachedTsRel uint32
we want to avoid any global variable, is that possible? At least move inside the Test function
and have a more descriptive name?
There was a problem hiding this comment.
Reviewed 4 of 13 files at r4.
Reviewable status: 6 of 15 files reviewed, 10 unresolved discussions (waiting on @mawyss)
go/lib/epic/doc.go, line 15 at r5 (raw file):
// limitations under the License. /*
use // instead, see
https://github.com/scionproto/scion/blob/master/go/pkg/router/bfd/doc.go
go/lib/epic/export_test.go, line 22 at r5 (raw file):
) func ExtractPacketLifetimeMs() uint16 {
you can avoid a function but have https://github.com/scionproto/scion/blob/master/go/lib/snet/export_test.go#L23 (same below)
go/lib/slayers/path/epic/epic.go, line 117 at r5 (raw file):
return 16 } return 16 + p.ScionRaw.Len()
16 should be a const
There was a problem hiding this comment.
Reviewable status: 0 of 15 files reviewed, 10 unresolved discussions (waiting on @karampok)
go/lib/epic/doc.go, line 15 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
use
//instead, see
https://github.com/scionproto/scion/blob/master/go/pkg/router/bfd/doc.go
Done.
go/lib/epic/export_test.go, line 22 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
you can avoid a function but have https://github.com/scionproto/scion/blob/master/go/lib/snet/export_test.go#L23 (same below)
Done.
go/lib/slayers/path/epic/epic.go, line 117 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
16 should be a const
Done.
go/pkg/router/dataplane.go, line 675 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
you can also drop the comments, as the code should be clear what it does (all comments within that function)
Done.
go/pkg/router/dataplane.go, line 689 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
The implementation of a function that returns (X,error) should (ideally) return a nil X only if err is not nil. And a not-nil X if err is nil.
In other words, if the err is nil you expect the X to be meaningful happy path.As of now, you hide what is happening under the
malformedPatherror string
Done.
go/pkg/router/dataplane.go, line 715 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
this value should be a constant inside the epic package
Done.
go/pkg/router/dataplane.go, line 721 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
This if/else needs to be refactored to avoid else using a switch statement https://golang.org/doc/effective_go.html#switch. It is more go idiomatic and better readable.
Also, I suggest that this if/else is hidden into a unique function that is inside the epic package
Done.
go/pkg/router/dataplane.go, line 730 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
There should be no todo. You can remove it and add later a PR about scmp. Also
// TODO(name):is the format we currently use
Done.
go/pkg/router/dataplane.go, line 857 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
please provide an implementation without else
Done.
go/pkg/router/dataplane_test.go, line 53 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
we want to avoid any global variable, is that possible? At least move inside the Test function
and have a more descriptive name?
Done.
There was a problem hiding this comment.
Reviewed 1 of 13 files at r1, 1 of 2 files at r6, 3 of 9 files at r7.
Reviewable status: 5 of 15 files reviewed, 4 unresolved discussions (waiting on @karampok and @mawyss)
go/lib/epic/epic.go, line 165 at r7 (raw file):
// is either the penultimate or the last hop of the path. func VerifyHVFIfNecessary(scionRaw *scion.Raw, auth []byte, epicpath *epic.EpicPath, s *slayers.SCION, timestamp uint32) (bool, error) {
What is the reason why you need to return a bool?
The return values are bool,error but I think it can only be error.
If err!=nil verification failed.
go/lib/epic/epic.go, line 207 at r7 (raw file):
} else { hvf = epicpath.PHVF }
a more go idiomatic way is
hvf = epicpath.PHVF
if last{
hvf=epicpath.LHVF
}
(unless there is a function call that has significant process overhead)
go/lib/slayers/path/mac.go, line 40 at r7 (raw file):
// FullMAC returns the full 16 bytes of the HopField MAC. func FullMAC(h hash.Hash, info *InfoField, hf *HopField) []byte {
The implementation of this function is the same as the implementation of the function above.
The fullMAC description and implementation seems not optimal.
Can we only use the MAC function? Do we even need that outside the tests?
go/lib/slayers/path/mac.go, line 55 at r7 (raw file):
// otherwise an error is returned. func VerifyMAC(expectedMac []byte, hf *HopField) error { if !bytes.Equal(hf.Mac[:6], expectedMac[:6]) {
We could easily avoid having a VerifyMAC as currently, the only thing is doing is !bytes.Equal().
See if you can drop it and how it looks.
go/pkg/router/dataplane.go, line 857 at r5 (raw file):
Previously, mawyss wrote…
Done.
Do you know if the following implementation is working?
var ok bool
p.path, ok = p.scionLayer.Path.(*scion.Raw)
if !ok {
// TODO(lukedirtwalker) parameter problem invalid path?
return processResult{}, malformedPath
}
if p.isSubheader {
epicpath, ok := p.scionLayer.Path.(*epic.EpicPath)
if !ok {
return processResult{}, malformedPath
}
p.path = epicpath.ScionRaw
}
if we not, why not?
There was a problem hiding this comment.
Reviewable status: 5 of 15 files reviewed, 5 unresolved discussions (waiting on @karampok and @mawyss)
go/pkg/router/dataplane.go, line 709 at r7 (raw file):
auth := p.cachedMac if len(auth) != libepic.AuthLength {
I can not see how that can fail?
The p.cachedMac is always 16 bytes since you return return h.Sum(nil)[:6]
There was a problem hiding this comment.
Reviewable status: 5 of 15 files reviewed, 5 unresolved discussions (waiting on @karampok)
go/lib/epic/epic.go, line 165 at r7 (raw file):
Previously, karampok (Konstantinos) wrote…
What is the reason why you need to return a bool?
The return values are
bool,errorbut I think it can only beerror.
Iferr!=nilverification failed.
Yes, this would work too, at least for now. The idea is that the error indicates whether the operations succeeded (e.g., the initialization and calculation of the MAC), while the bool indicates (in case that there was no error) whether the calculated HVF corresponds to the HVF in the packet. This means that even if there was no error, the bool can be false. In the future this allows to send back SCMP packets when the HVF verification failed.
But I will gladly change it if you think it would make sense.
go/lib/epic/epic.go, line 207 at r7 (raw file):
Previously, karampok (Konstantinos) wrote…
a more go idiomatic way is
hvf = epicpath.PHVF if last{ hvf=epicpath.LHVF }(unless there is a function call that has significant process overhead)
Done.
go/lib/slayers/path/mac.go, line 40 at r7 (raw file):
Previously, karampok (Konstantinos) wrote…
The implementation of this function is the same as the implementation of the function above.
The fullMAC description and implementation seems not optimal.
Can we only use theMACfunction? Do we even need that outside the tests?
Yes, the implementation is the same, with the only difference that FullMAC() returns 16 bytes and not only 6 bytes.
This is important because EPIC-HP needs the full 16 bytes to calculate the PHVF /LHVF.
I changed the description to make this more clear.
Initially I planned to change the MAC() function to return 16 bytes instead of 6. But this would affect > 20 files (mostly in /go/integration however), and I tried to avoid larger changes, especially if it affects a core mechanism like the MAC calculation. But please let me know if you want this to be changed.
go/lib/slayers/path/mac.go, line 55 at r7 (raw file):
Previously, karampok (Konstantinos) wrote…
We could easily avoid having a VerifyMAC as currently, the only thing is doing is
!bytes.Equal().
See if you can drop it and how it looks.
I checked that now. I assume that the creator of this function did not want to rewrite the error output (expected vs. calculated MAC) everywhere this MAC verification is needed. This would lead to fairly long error messages (see verifyCurrentMAC() in dataplane.go).
Do you want me to change it anyway?
go/pkg/router/dataplane.go, line 857 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
Do you know if the following implementation is working?
var ok bool p.path, ok = p.scionLayer.Path.(*scion.Raw) if !ok { // TODO(lukedirtwalker) parameter problem invalid path? return processResult{}, malformedPath } if p.isSubheader { epicpath, ok := p.scionLayer.Path.(*epic.EpicPath) if !ok { return processResult{}, malformedPath } p.path = epicpath.ScionRaw }if we not, why not?
I don't think this works:
p.scionLayer.Path is either a pointer to a scion.Raw or epic.EpicPath. In the case of epic.EpicPath, the first assignment fails (ok = false), and the conditional branching on p.isSubheader will never be reached.
(I also checked it, with this implementation the router tests start failing with a malformedPath exception)
go/pkg/router/dataplane.go, line 709 at r7 (raw file):
Previously, karampok (Konstantinos) wrote…
I can not see how that can fail?
Thep.cachedMacis always 16 bytes since you returnreturn h.Sum(nil)[:6]
You are right, and it is also checked later in VerifyHVF(). Removed this check here.
There was a problem hiding this comment.
Reviewable status: 5 of 15 files reviewed, 4 unresolved discussions (waiting on @karampok and @mawyss)
go/lib/epic/epic.go, line 165 at r7 (raw file):
Previously, mawyss wrote…
Yes, this would work too, at least for now. The idea is that the error indicates whether the operations succeeded (e.g., the initialization and calculation of the MAC), while the bool indicates (in case that there was no error) whether the calculated HVF corresponds to the HVF in the packet. This means that even if there was no error, the bool can be false. In the future this allows to send back SCMP packets when the HVF verification failed.
But I will gladly change it if you think it would make sense.
okay, let's keep it simple and return only error. When we add functionality that requires error handling and separate ok value we can refactor.
go/lib/slayers/path/mac.go, line 40 at r7 (raw file):
Previously, mawyss wrote…
Yes, the implementation is the same, with the only difference that FullMAC() returns 16 bytes and not only 6 bytes.
This is important because EPIC-HP needs the full 16 bytes to calculate the PHVF /LHVF.
I changed the description to make this more clear.Initially I planned to change the MAC() function to return 16 bytes instead of 6. But this would affect > 20 files (mostly in /go/integration however), and I tried to avoid larger changes, especially if it affects a core mechanism like the MAC calculation. But please let me know if you want this to be changed.
okay, I need to discuss that.
For the time being, please provide and implementation that the MAC function is re-using FullMac
func MAC() ...{
return FullMAC[:6]
}
go/lib/slayers/path/mac.go, line 55 at r7 (raw file):
Previously, mawyss wrote…
I checked that now. I assume that the creator of this function did not want to rewrite the error output (expected vs. calculated MAC) everywhere this MAC verification is needed. This would lead to fairly long error messages (see verifyCurrentMAC() in dataplane.go).
Do you want me to change it anyway?
I think it is being used only in two places, you can remove the function.
go/pkg/router/dataplane.go, line 857 at r5 (raw file):
Previously, mawyss wrote…
I don't think this works:
p.scionLayer.Path is either a pointer to a scion.Raw or epic.EpicPath. In the case of epic.EpicPath, the first assignment fails (ok = false), and the conditional branching on p.isSubheader will never be reached.(I also checked it, with this implementation the router tests start failing with a malformedPath exception)
Have you tried something like
C path type header.
type EpicPath struct {
scion.Raw
PacketTimestamp uint64
PHVF []byte
LHVF []byte
}
no sure if it is the right way
go/pkg/router/dataplane.go, line 820 at r8 (raw file):
func (p *scionPacketProcessor) parsePath() (processResult, error) { switch p.isSubheader {
Subheader is no ideal, let's have is isEpic
There was a problem hiding this comment.
Reviewable status: 5 of 15 files reviewed, 4 unresolved discussions (waiting on @karampok)
go/lib/epic/epic.go, line 165 at r7 (raw file):
Previously, karampok (Konstantinos) wrote…
okay, let's keep it simple and return only error. When we add functionality that requires error handling and separate ok value we can refactor.
Done.
go/lib/slayers/path/mac.go, line 40 at r7 (raw file):
Previously, karampok (Konstantinos) wrote…
okay, I need to discuss that.
For the time being, please provide and implementation that theMACfunction is re-usingFullMacfunc MAC() ...{ return FullMAC[:6] }
Done.
go/lib/slayers/path/mac.go, line 55 at r7 (raw file):
Previously, karampok (Konstantinos) wrote…
I think it is being used only in two places, you can remove the function.
Done.
go/pkg/router/dataplane.go, line 857 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
Have you tried something like
C path type header. type EpicPath struct { scion.Raw PacketTimestamp uint64 PHVF []byte LHVF []byte }no sure if it is the right way
Ah ok, I see now what you meant before. But I still don't think that this approach will work:
In case the p.scionLayer.Path is a pointer to an epic.EpicPath, then the first assignment will still return (, false).
In particular, Go's type assertion seems not to allow those kind of assertions to struct compositions.
I also verified this behavior in form of a simplified example in the Go Playground (https://play.golang.org/).
go/pkg/router/dataplane.go, line 820 at r8 (raw file):
Previously, karampok (Konstantinos) wrote…
Subheader is no ideal, let's have is
isEpic
Done.
There was a problem hiding this comment.
Reviewed 1 of 13 files at r4, 1 of 2 files at r6, 3 of 9 files at r7, 4 of 5 files at r9, 1 of 1 files at r10.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mawyss)
go/lib/slayers/path/epic/epic.go, line 59 at r10 (raw file):
return serrors.New("epic path must not be nil") } if len(b) < 16 {
Ideally, all the int values in this file should be on top on a constant (not exported)
go/pkg/router/dataplane.go, line 857 at r5 (raw file):
Previously, mawyss wrote…
Ah ok, I see now what you meant before. But I still don't think that this approach will work:
In case the p.scionLayer.Path is a pointer to an epic.EpicPath, then the first assignment will still return (, false).
In particular, Go's type assertion seems not to allow those kind of assertions to struct compositions.
I also verified this behavior in form of a simplified example in the Go Playground (https://play.golang.org/).
Ok, in that case, I have to ask to revert to a if else since the final switch on just on false/true seems a bit ackword. Sorry for that.
There was a problem hiding this comment.
Reviewable status: 12 of 15 files reviewed, 1 unresolved discussion (waiting on @karampok)
go/lib/slayers/path/epic/epic.go, line 59 at r10 (raw file):
Previously, karampok (Konstantinos) wrote…
Ideally, all the int values in this file should be on top on a constant (not exported)
Done. Except for one occurrence of "return 0", all other values are now constants.
go/pkg/router/dataplane.go, line 857 at r5 (raw file):
Previously, karampok (Konstantinos) wrote…
Ok, in that case, I have to ask to revert to a
if elsesince the final switch on just on false/true seems a bit ackword. Sorry for that.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r11, 2 of 2 files at r12.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @mawyss)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 36 unresolved discussions (waiting on @mawyss and @oncilla)
go/lib/epic/epic.go, line 34 at r12 (raw file):
const ( // authLength denotes the size of the authenticator in bytes authLength = 16
AuthLen
go/lib/epic/epic.go, line 36 at r12 (raw file):
authLength = 16 // packetLifetimeMs denotes the maximal lifetime of a packet in milliseconds packetLifetimeMs uint16 = 2000
MaxPacketLifetime
go/lib/epic/epic.go, line 38 at r12 (raw file):
packetLifetimeMs uint16 = 2000 // clockSkewMs denotes the maximal clock skew in milliseconds clockSkewMs uint16 = 1000
MaxClockSkew or MaxClockSkewMilliseconds
go/lib/epic/epic.go, line 42 at r12 (raw file):
// CreateEpicTimestamp creates the EPIC packetTimestamp from tsRel, coreID, and coreCounter. func CreateEpicTimestamp(tsRel uint32, coreID uint8, coreCounter uint32) (packetTimestamp uint64) {
We don't use named return arguments in the code base. Please fix also in other places.
go/lib/epic/epic.go, line 42 at r12 (raw file):
// CreateEpicTimestamp creates the EPIC packetTimestamp from tsRel, coreID, and coreCounter. func CreateEpicTimestamp(tsRel uint32, coreID uint8, coreCounter uint32) (packetTimestamp uint64) {
Looking at this again, I find Timestamp not to be a suitable naming for the Epic Timestamp. More generically, it is a packet ID that contains a relative timestamp and a counter. Thus I propose you create something like
type PktID struct {
Timestamp uint32
CoreID uint8
CoreCounter uint32
}
func (i *PktID) DecodeFromBytes(raw []byte) {
i.Timestamp = binary.BigEndian.Uint32(raw[:4])
coreInfo := binary.BigEndian.Uint32(raw[4:8])
i.CoreID = uint8(coreInfo >> 24)
i.CoreCounter = coreInfo & 0x00FFFFFF
}
func (i *PktID) SerializeTo(b []byte) {
binary.BigEndian.PutUint32(b[:4], i.Timestamp)
coreInfo := uint32(i.CoreID)<<24 | i.CoreCounter
binary.BigEndian.PutUint32(b[4:8], coreInfo)
}
and work with that.
Then, Timestamp would really only refer to what is now called TsRel
go/lib/epic/epic.go, line 59 at r12 (raw file):
// CreateEpicTimestampCustom creates the EPIC packetTimestamp from tsRel and pckId. func CreateEpicTimestampCustom(tsRel uint32, pckId uint32) (packetTimestamp uint64) {
Would drop this function as it's not needed.
go/lib/epic/epic.go, line 88 at r12 (raw file):
// time. It must not be in the future, otherwise an error is returned. The current time must be at // most 1 day and 63 minutes after the input timestamp, otherwise an error is returned. func CreateTsRel(timestamp uint32) (uint32, error) {
This would now be called CreateTimestamp
go/lib/epic/epic.go, line 88 at r12 (raw file):
timestamp
I'd call this input
go/lib/epic/epic.go, line 95 at r12 (raw file):
return 0, serrors.New("provided timestamp is in the future", "timestamp", tsMicro, "now", nowMicro) } else {
No need for the else since you return in the error case
Also, diff is not really needed, since it's only used in one expression.
go/lib/epic/epic.go, line 102 at r12 (raw file):
tsRel := max(0, (diff/21)-1) if tsRel >= (1 << 32) { return 0, serrors.New("current time is more than 1 day"+
serrors.New("diff between input and now >1d63min", "diff", ...)
Also, translate it to something that is readable by a human instead of milliseconds.
go/lib/epic/epic.go, line 131 at r12 (raw file):
(nowMs > tsSenderMs+uint64(packetLifetimeMs)+uint64(clockSkewMs)) { return false } else {
drop else since you return anyway in the other branch
go/lib/epic/epic.go, line 138 at r12 (raw file):
// CalculateEpicMac derives the EPIC MAC (PHVF/LHVF) given the full 16 bytes of the SCION path type // MAC (auth), the EPIC path type header (epicpath), and the SCION common/address header (s). func CalculateEpicMac(auth []byte, epicpath *epic.EpicPath, s *slayers.SCION,
Should be called CalcMac -> reads then as epic.CalcMac()
go/lib/epic/epic.go, line 151 at r12 (raw file):
return nil, err } if len(input) < 16 || len(input)%16 != 0 {
You are in full control how the input is generated. No need for this check. If anything that goes into the MAC calculation is wrong it should be caught by prepareMacInput
go/lib/epic/epic.go, line 158 at r12 (raw file):
16
This should be f.BlockSize()
go/lib/epic/epic.go, line 163 at r12 (raw file):
// VerifyHVFIfNecessary verifies the correctness of the HVF if necessary, i.e., if the current hop // is either the penultimate or the last hop of the path. func VerifyHVFIfNecessary(scionRaw *scion.Raw, auth []byte, epicpath *epic.EpicPath,
Drop this method.
go/lib/epic/epic.go, line 190 at r12 (raw file):
// length, or if the MAC calculation gives an error, also VerifyHVF returns an error. The // verification was successful if and only if VerifyHVF returns nil. func VerifyHVF(auth []byte, epicpath *epic.EpicPath, s *slayers.SCION,
instead of the last bool, simply pass in either the PHVF or the LHVF
go/lib/epic/epic.go, line 216 at r12 (raw file):
// isPenultimateHop returns whether the current hop is the penultimate hop on the path. // It returns an error if scionRaw is nil. func isPenultimateHop(scionRaw *scion.Raw) (bool, error) {
Either define this as a method on scion.Raw or return false in case scionRaw is nil. Then you don't have to have an error return value. Makes for less boilerplate.
Same for the next Method.
go/lib/epic/epic.go, line 249 at r12 (raw file):
} func inputToBytes(timestamp uint32, packetTimestamp uint64,
This can be rolled into prepareMacInput
go/lib/epic/epic.go, line 290 at r12 (raw file):
return nil, serrors.New("SCION common+address header must not be nil") } packetTimestamp := epicpath.PacketTimestamp
Don't define these locals here. You never use them except passing them to a method.
go/lib/slayers/path/epic/epic.go, line 32 at r12 (raw file):
// overhead denotes the number of bytes the EPIC path type contains in addition to the SCION // path type. It is the sum of the PacketTimestamp (8B), the PHVF (4B) and the LHVF (4B) sizes. overhead = 16
Maybe call this MetadataLen?
go/lib/slayers/path/epic/epic.go, line 34 at r12 (raw file):
overhead = 16 // lenPckTs denotes the length of the packet timestamp. lenPckTs = 8
PktIDLen
go/lib/slayers/path/epic/epic.go, line 37 at r12 (raw file):
// lenHVF denotes the length of the hop validation fields. The length is the same for both the // PHVF and the LHVF. lenHVF = 4
HVFLen
go/lib/slayers/path/epic/epic.go, line 52 at r12 (raw file):
// EpicPath denotes the EPIC path type header. type EpicPath struct {
This should just be Path -> epic.Path as opposed to epic.EpicPath
go/lib/slayers/path/epic/epic.go, line 53 at r12 (raw file):
// EpicPath denotes the EPIC path type header. type EpicPath struct { PacketTimestamp uint64
This would be PktID epic.PktID
go/lib/slayers/path/epic/epic.go, line 56 at r12 (raw file):
PHVF []byte LHVF []byte ScionRaw *scion.Raw
ScionPath
go/lib/slayers/path/epic/epic.go, line 62 at r12 (raw file):
// SerializeTo will return nil. func (p *EpicPath) SerializeTo(b []byte) error { if p == nil {
You don't need to handle nil in the callee. Let the caller do this. Same for other places.
go/lib/slayers/path/epic/epic.go, line 65 at r12 (raw file):
return serrors.New("epic path must not be nil") } if len(b) < overhead {
This should be
if len(b) < p.Len() {
return serrors.New("buffer too small to serialize path.", "expected", p.Len(),
"actual", len(b))
}
go/lib/slayers/path/epic/epic.go, line 69 at r12 (raw file):
strconv.Itoa(overhead) + " bytes)") } if len(p.PHVF) != lenHVF || len(p.LHVF) != lenHVF {
Handle these separately. Also, a more appropriate error message would be
serrors.New("invalid length of PHVF", "expected", HVFLen, "actual", len(p.PHVF)
go/lib/slayers/path/epic/epic.go, line 74 at r12 (raw file):
} if p.ScionRaw == nil { return serrors.New("scion subheader must exist")
"SCION path is nil"
go/lib/slayers/path/epic/epic.go, line 89 at r12 (raw file):
} if len(b) < overhead { return serrors.New("EpicPath bytes too short (< " + strconv.Itoa(overhead) + " bytes)")
serrors.New("EpicPath raw too short", "expected", MetadataLen, "actual", len(b))
go/pkg/router/dataplane.go, line 690 at r12 (raw file):
timestamp := info.Timestamp packetTimestamp := epicpath.PacketTimestamp libepic.VerifyTimestamp(timestamp, packetTimestamp)
You must handle the result
go/pkg/router/dataplane.go, line 703 at r12 (raw file):
result, err := p.process() if err != nil { return processResult{}, err
You cannot simply discard the result of p.process()...
go/pkg/router/dataplane.go, line 705 at r12 (raw file):
return processResult{}, err } auth := p.cachedMac
Not needed. You can pass it directly to the method.
go/pkg/router/dataplane.go, line 707 at r12 (raw file):
auth := p.cachedMac err = libepic.VerifyHVFIfNecessary(scionRaw, auth, epicpath, &s, timestamp)
This can be done in one line
go/pkg/router/dataplane.go, line 709 at r12 (raw file):
err = libepic.VerifyHVFIfNecessary(scionRaw, auth, epicpath, &s, timestamp) if err != nil { return processResult{}, err
This should create a SCMP reply.
go/pkg/router/dataplane.go, line 731 at r12 (raw file):
buffer gopacket.SerializeBuffer // isEpic indicates whether the scion path is part of an epic-hp path. isEpic bool
Instead of introducing this flag, you can set the path on the scionPacketProcessor before you call p.process() and remove the path extraction code from processPath() and put it to processSCION and processEPIC instead.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 52 unresolved discussions (waiting on @mawyss and @oncilla)
go/lib/epic/epic.go, line 114 at r12 (raw file):
// a possible clock drift between the packet source and the verifier of up to one second into // account. func VerifyTimestamp(timestamp uint32, packetTimestamp uint64) bool {
This should return an error instead of simply true or false.
go/lib/epic/epic.go, line 138 at r12 (raw file):
epicpath *epic.EpicPath
You don't need the entire path, only the PktID is needed (former timestamp)
go/lib/epic/epic.go, line 190 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
instead of the
lastbool, simply pass in either thePHVFor theLHVF
Also, you don't need the entire path. Only the PktID is needed here plus either the PHVF or LHVF
go/lib/epic/epic.go, line 209 at r12 (raw file):
err = nil if !bytes.Equal(hvf, mac) { err = serrors.New("epic hop validation field verification failed")
return serrors.New(...)
Also include the values as context.
go/lib/epic/epic.go, line 211 at r12 (raw file):
err = serrors.New("epic hop validation field verification failed") } return err
return nil
go/lib/slayers/path/epic/epic_test.go, line 52 at r12 (raw file):
) func TestSerializeDecode(t *testing.T) {
Please also add tests for serialize and decode alone, as well as a test for reverse.
go/pkg/router/dataplane.go, line 690 at r12 (raw file):
timestamp := info.Timestamp packetTimestamp := epicpath.PacketTimestamp libepic.VerifyTimestamp(timestamp, packetTimestamp)
Why do you verify the timestamp on each hop? Wouldn't just the last two hops need to do that?
go/pkg/router/dataplane.go, line 961 at r12 (raw file):
6
All these 6's should be path.MacLen (defined in hopfield.go)
go/pkg/router/dataplane.go, line 967 at r12 (raw file):
}, &slayers.SCMPParameterProblem{Pointer: p.currentHopPointer()}, serrors.New("MAC", "expected", fmt.Sprintf("%x", fullMac[:6]),
Change this to MAC verification failed
go/pkg/router/dataplane.go, line 1255 at r12 (raw file):
// OHP leaving our IA if d.localIA.Equal(s.SrcIA) { fullMac := path.FullMAC(d.macFactory(), &p.Info, &p.FirstHop)
This can just use path.MAC()
go/lib/epic/epic_test.go, line 33 at r12 (raw file):
) func TestMacInputGeneration(t *testing.T) {
Add a test for when the scion path would be nil
go/lib/epic/epic_test.go, line 46 at r12 (raw file):
} func TestTimestamp(t *testing.T) {
This test only tests that CreateEpicTimestamp is the reverse of ParseEpicTimestamp but not that they are correct. You should test them individually.
go/lib/epic/epic_test.go, line 58 at r12 (raw file):
} func TestTsRel(t *testing.T) {
This doesn't test the actual timestamp that is created, but only whether an error is returned or not. Please also check the actual timestamps.
Also, please convert to table driven tests with test names.
go/lib/epic/epic_test.go, line 87 at r12 (raw file):
csAndPl := uint32(libepic.ClockSkewMs + libepic.PacketLifetimeMs) testCases := map[uint32]bool{
These test cases are hard to figure out what they do. Please follow our established pattern where
testCases := map[string]struct{
input uint32,
expected bool
}{}
go/lib/epic/epic_test.go, line 104 at r12 (raw file):
} func TestHVFVerification(t *testing.T) {
Please convert to table driven tests with test names. Make it much easier to verify (and later debug) what's going on.
go/lib/epic/epic_test.go, line 178 at r12 (raw file):
createScionPath(1, 2): true, createScionPath(2, 2): false, createScionPath(0, -1): false,
Negative number of hops?
go/lib/epic/epic_test.go, line 222 at r12 (raw file):
} func randUint64() uint64 {
There is rand.Uint64()
There was a problem hiding this comment.
Reviewable status: 6 of 17 files reviewed, 52 unresolved discussions (waiting on @karampok, @mawyss, @oncilla, and @shitz)
go/lib/epic/epic.go, line 34 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
AuthLen
Done.
go/lib/epic/epic.go, line 36 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
MaxPacketLifetime
Done.
go/lib/epic/epic.go, line 38 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
MaxClockSkeworMaxClockSkewMilliseconds
Done.
go/lib/epic/epic.go, line 42 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
We don't use named return arguments in the code base. Please fix also in other places.
Done.
go/lib/epic/epic.go, line 42 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Looking at this again, I find
Timestampnot to be a suitable naming for the Epic Timestamp. More generically, it is a packet ID that contains a relative timestamp and a counter. Thus I propose you create something liketype PktID struct { Timestamp uint32 CoreID uint8 CoreCounter uint32 } func (i *PktID) DecodeFromBytes(raw []byte) { i.Timestamp = binary.BigEndian.Uint32(raw[:4]) coreInfo := binary.BigEndian.Uint32(raw[4:8]) i.CoreID = uint8(coreInfo >> 24) i.CoreCounter = coreInfo & 0x00FFFFFF } func (i *PktID) SerializeTo(b []byte) { binary.BigEndian.PutUint32(b[:4], i.Timestamp) coreInfo := uint32(i.CoreID)<<24 | i.CoreCounter binary.BigEndian.PutUint32(b[4:8], coreInfo) }and work with that.
Then,
Timestampwould really only refer to what is now calledTsRel
Done. I added the code to the epic.go file from the slayers folder, because your code only does parsing.
go/lib/epic/epic.go, line 59 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Would drop this function as it's not needed.
Done. (I added this function because the EPIC specification mentions that the combination of CoreID and CoreCounter is only a suggestion, but the end hosts could use their own identifier)
go/lib/epic/epic.go, line 88 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This would now be called
CreateTimestamp
Done.
go/lib/epic/epic.go, line 88 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
timestampI'd call this
input
Done.
go/lib/epic/epic.go, line 95 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
No need for the
elsesince you return in the error caseAlso,
diffis not really needed, since it's only used in one expression.
Done. (I kept diff now because it is also used in the error)
go/lib/epic/epic.go, line 102 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
serrors.New("diff between input and now >1d63min", "diff", ...)Also, translate it to something that is readable by a human instead of milliseconds.
Done.
go/lib/epic/epic.go, line 114 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This should return an error instead of simply true or false.
Done.
go/lib/epic/epic.go, line 131 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
drop
elsesince you return anyway in the other branch
Done.
go/lib/epic/epic.go, line 138 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Should be called
CalcMac-> reads then asepic.CalcMac()
Done.
go/lib/epic/epic.go, line 138 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
epicpath *epic.EpicPathYou don't need the entire path, only the
PktIDis needed (former timestamp)
Done.
go/lib/epic/epic.go, line 151 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
You are in full control how the input is generated. No need for this check. If anything that goes into the MAC calculation is wrong it should be caught by
prepareMacInput
Done.
go/lib/epic/epic.go, line 158 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
16This should be f.BlockSize()
Done.
go/lib/epic/epic.go, line 163 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Drop this method.
Done.
go/lib/epic/epic.go, line 190 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Also, you don't need the entire path. Only the
PktIDis needed here plus either the PHVF or LHVF
Done.
go/lib/epic/epic.go, line 209 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
return
serrors.New(...)
Also include the values as context.
Done.
go/lib/epic/epic.go, line 211 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
return nil
Done.
go/lib/epic/epic.go, line 216 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Either define this as a method on
scion.Rawor returnfalsein casescionRawis nil. Then you don't have to have an error return value. Makes for less boilerplate.Same for the next Method.
Done. (Defined it as method on scion.Raw)
go/lib/epic/epic.go, line 249 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This can be rolled into
prepareMacInput
Done.
go/lib/epic/epic.go, line 290 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Don't define these locals here. You never use them except passing them to a method.
Done.
go/lib/slayers/path/epic/epic.go, line 32 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Maybe call this
MetadataLen?
Done.
go/lib/slayers/path/epic/epic.go, line 34 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
PktIDLen
Done.
go/lib/slayers/path/epic/epic.go, line 37 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
HVFLen
Done.
go/lib/slayers/path/epic/epic.go, line 52 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This should just be
Path->epic.Pathas opposed toepic.EpicPath
Done.
go/lib/slayers/path/epic/epic.go, line 53 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This would be
PktID epic.PktID
Done.
go/lib/slayers/path/epic/epic.go, line 56 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
ScionPath
Done.
go/lib/slayers/path/epic/epic.go, line 62 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
You don't need to handle nil in the callee. Let the caller do this. Same for other places.
Done. Thank you for the hint, I did not know so I went for the safer way.
go/lib/slayers/path/epic/epic.go, line 65 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This should be
if len(b) < p.Len() { return serrors.New("buffer too small to serialize path.", "expected", p.Len(), "actual", len(b)) }
Done.
go/lib/slayers/path/epic/epic.go, line 69 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Handle these separately. Also, a more appropriate error message would be
serrors.New("invalid length of PHVF", "expected", HVFLen, "actual", len(p.PHVF)
Done.
go/lib/slayers/path/epic/epic.go, line 74 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
"SCION path is nil"
Done.
go/lib/slayers/path/epic/epic.go, line 89 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
serrors.New("EpicPath raw too short", "expected", MetadataLen, "actual", len(b))
Done.
go/lib/slayers/path/epic/epic_test.go, line 52 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Please also add tests for serialize and decode alone, as well as a test for reverse.
Done.
go/pkg/router/dataplane.go, line 690 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
You must handle the result
Done.
go/pkg/router/dataplane.go, line 690 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Why do you verify the timestamp on each hop? Wouldn't just the last two hops need to do that?
The idea is to block expired packets (timestamp in the past) already at the first hop(s), instead of letting the packet traverse the whole path until it is discarded by the last two hops only. But I understand if this is undesirable, as EPIC-HP should behave like SCION, except for the last two hops. Please let me know if you think it makes more sense to have it checked only at the penultimate and last hops.
go/pkg/router/dataplane.go, line 705 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Not needed. You can pass it directly to the method.
Done.
go/pkg/router/dataplane.go, line 707 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This can be done in one line
Done. (I removed the VerifyHVFIfNecessary function, but I used VerifyHVF in only one line, as suggested in your comment)
go/pkg/router/dataplane.go, line 731 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Instead of introducing this flag, you can set the path on the
scionPacketProcessorbefore you callp.process()and remove the path extraction code fromprocessPath()and put it toprocessSCIONandprocessEPICinstead.
Done.
go/pkg/router/dataplane.go, line 961 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
6All these 6's should be
path.MacLen(defined inhopfield.go)
Done.
go/pkg/router/dataplane.go, line 967 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Change this to
MAC verification failed
Done.
go/pkg/router/dataplane.go, line 1255 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This can just use
path.MAC()
Done.
go/lib/epic/epic_test.go, line 33 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Add a test for when the scion path would be nil
Done.
go/lib/epic/epic_test.go, line 46 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This test only tests that
CreateEpicTimestampis the reverse ofParseEpicTimestampbut not that they are correct. You should test them individually.
Done. (Moved them to the epic_test.go from slayers, because this is where the PktID struct is now)
go/lib/epic/epic_test.go, line 58 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This doesn't test the actual timestamp that is created, but only whether an error is returned or not. Please also check the actual timestamps.
Also, please convert to table driven tests with test names.
Done. Please note that I added a further input to CreateTimestamp, namely the current time in nanoseconds. This allows to make the unit tests independent of their execution time.
go/lib/epic/epic_test.go, line 87 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
These test cases are hard to figure out what they do. Please follow our established pattern where
testCases := map[string]struct{ input uint32, expected bool }{}
Done. I added an additional argument to VerifyTimestamp, namely nowNanoseconds, which allows to make the tests independent of their execution time. Furthermore I changed the precision of MaxClockSkew and MaxPacketLifetime to microseconds (instead of milliseconds), because this is the precision of the EPIC timestamp anyway, and this also makes the code cleaner.
go/lib/epic/epic_test.go, line 104 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Please convert to table driven tests with test names. Make it much easier to verify (and later debug) what's going on.
Done.
go/lib/epic/epic_test.go, line 178 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Negative number of hops?
Done.
go/lib/epic/epic_test.go, line 222 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
There is
rand.Uint64()
Done. Removed the function as it is not needed anymore.
There was a problem hiding this comment.
Reviewed 7 of 11 files at r13.
Reviewable status: 13 of 17 files reviewed, 23 unresolved discussions (waiting on @karampok, @mawyss, @oncilla, and @shitz)
go/lib/epic/epic.go, line 59 at r12 (raw file):
Previously, mawyss wrote…
Done. (I added this function because the EPIC specification mentions that the combination of CoreID and CoreCounter is only a suggestion, but the end hosts could use their own identifier)
see my comment about the epic.PktID below
go/lib/epic/epic.go, line 45 at r13 (raw file):
// is returned. The current time must be at most 1 day and 63 minutes after the input timestamp, // otherwise an error is returned. func CreateTimestamp(input uint32, nowNanoseconds int64) (uint32, error) {
It would be nicer to work with the time package
func CreateTimestamp(input time.Time, now time.Time) (uint32, error) {
if input.After(now) {
return 0, serrors.New("provided input timestamp is in the future",
"input", input, "now", now)
}
diff := now.Sub(input).Microseconds()
tsRel := diff/21 - 1
if tsRel < 0 {
tsRel = 0
}
if tsRel >= (1 << 32) {
return 0, serrors.New("diff between input and now >1d63min",
"diff", diff)
}
return uint32(tsRel), nil
}
go/lib/epic/epic.go, line 68 at r13 (raw file):
// a possible clock drift between the packet source and the verifier of up to one second into // account. func VerifyTimestamp(timestamp uint32, packetTimestamp uint32, nowNanoseconds int64) error {
This would also be nicer if it would work with the time package.
go/lib/epic/epic.go, line 93 at r13 (raw file):
pktID *epic.PktID
Pass the pktID by value (it's fast). Then you can save yourself some nil-checks
go/lib/epic/epic.go, line 117 at r13 (raw file):
pktID *epic.PktID
pass by value
go/lib/epic/epic.go, line 181 at r13 (raw file):
// Fill input input[0] = srcAddrLen binary.BigEndian.PutUint32(input[1:5], timestamp)
Instead of these magic numbers it's cleaner if you use an offset counter
(see for example https://github.com/scionproto/scion/blob/master/go/lib/slayers/scion.go#L345)
go/lib/epic/epic.go, line 183 at r13 (raw file):
binary.BigEndian.PutUint64(input[13:21], uint64(s.SrcIA.A))
The IA can be written to bytes as
s.SrcIA.Write(input[13:21])
go/lib/slayers/path/epic/epic.go, line 62 at r12 (raw file):
Previously, mawyss wrote…
Done. Thank you for the hint, I did not know so I went for the safer way.
Not done?
go/lib/slayers/path/epic/epic.go, line 130 at r13 (raw file):
// PktID denotes the EPIC packet ID. type PktID struct {
Actually, I have a better suggestion that is more in line with the specification.
type PktID struct {
Timestamp uint32
Counter uint32
}
and then in go/lib/epic.go define helper methods
func PktCounterFromCore(uint8 coreID, uint32 coreCounter) uint32
func CoreFromPktCounter(counter uint32) (uint8, uint32)
that way we leave the epic.PktID agnostic to the way the Counter is generated, but offer helper methods for one of the proposed procedures in the design doc.
go/pkg/router/dataplane.go, line 690 at r12 (raw file):
Previously, mawyss wrote…
The idea is to block expired packets (timestamp in the past) already at the first hop(s), instead of letting the packet traverse the whole path until it is discarded by the last two hops only. But I understand if this is undesirable, as EPIC-HP should behave like SCION, except for the last two hops. Please let me know if you think it makes more sense to have it checked only at the penultimate and last hops.
I'd prefer if only the last two border routers will treat the EPIC portion of a path. Sure, replays might travel a bit further, but in the benign (common) case intermediate routers have to do less work.
go/pkg/router/dataplane.go, line 716 at r13 (raw file):
} if scionPath.IsPenultimateHop() {
I'd slightly prefer
var hvf []byte
if scionPath.IsPenultimateHop() {
hvf = path.PHVF
} else if scionPath.IsLastHop() {
hvf = path.LHVF
} else {
return result, nil
}
if err := libepic.VerifyHVF(p.cachedMac, &path.PktID, &s, timestamp, hvf); err != nil {
return processResult{}, err
}
return result, nil
(should create an SCMP reply of course)
go/pkg/router/dataplane.go, line 1256 at r13 (raw file):
6
path.MacLen here and below
There was a problem hiding this comment.
Reviewable status: 7 of 17 files reviewed, 23 unresolved discussions (waiting on @karampok, @oncilla, and @shitz)
go/lib/epic/epic.go, line 59 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
see my comment about the
epic.PktIDbelow
Done.
go/lib/epic/epic.go, line 45 at r13 (raw file):
Previously, shitz (Samuel Hitz) wrote…
It would be nicer to work with the
timepackagefunc CreateTimestamp(input time.Time, now time.Time) (uint32, error) { if input.After(now) { return 0, serrors.New("provided input timestamp is in the future", "input", input, "now", now) } diff := now.Sub(input).Microseconds() tsRel := diff/21 - 1 if tsRel < 0 { tsRel = 0 } if tsRel >= (1 << 32) { return 0, serrors.New("diff between input and now >1d63min", "diff", diff) } return uint32(tsRel), nil }
Done.
go/lib/epic/epic.go, line 68 at r13 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This would also be nicer if it would work with the
timepackage.
Done.
go/lib/epic/epic.go, line 93 at r13 (raw file):
Previously, shitz (Samuel Hitz) wrote…
pktID *epic.PktIDPass the
pktIDby value (it's fast). Then you can save yourself some nil-checks
Done.
go/lib/epic/epic.go, line 117 at r13 (raw file):
Previously, shitz (Samuel Hitz) wrote…
pktID *epic.PktIDpass by value
Done.
go/lib/epic/epic.go, line 181 at r13 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Instead of these magic numbers it's cleaner if you use an offset counter
(see for example https://github.com/scionproto/scion/blob/master/go/lib/slayers/scion.go#L345)
Done.
go/lib/epic/epic.go, line 183 at r13 (raw file):
Previously, shitz (Samuel Hitz) wrote…
binary.BigEndian.PutUint64(input[13:21], uint64(s.SrcIA.A))The IA can be written to bytes as
s.SrcIA.Write(input[13:21])
Done.
go/lib/slayers/path/epic/epic.go, line 62 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Not done?
Done :)
go/lib/slayers/path/epic/epic.go, line 130 at r13 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Actually, I have a better suggestion that is more in line with the specification.
type PktID struct { Timestamp uint32 Counter uint32 }and then in
go/lib/epic.godefine helper methodsfunc PktCounterFromCore(uint8 coreID, uint32 coreCounter) uint32 func CoreFromPktCounter(counter uint32) (uint8, uint32)that way we leave the
epic.PktIDagnostic to the way theCounteris generated, but offer helper methods for one of the proposed procedures in the design doc.
Done.
go/pkg/router/dataplane.go, line 690 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
I'd prefer if only the last two border routers will treat the EPIC portion of a path. Sure, replays might travel a bit further, but in the benign (common) case intermediate routers have to do less work.
Done
go/pkg/router/dataplane.go, line 703 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
You cannot simply discard the result of
p.process()...
I return processResult{} because I want to be sure the packet is dropped, I planned to implement SCMP responses in a separate PR. Please see my other comment regarding SCMP replies.
go/pkg/router/dataplane.go, line 709 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This should create a SCMP reply.
If I understand correctly, the border router can currently only return SCMP packets for SCION path type packets, not for arbitrary path types: prepareSCMP() (dataplane.go) takes the path header as path := s.scionL.Path.(*scion.Raw).
To implement EPIC SCMP replies I would go in the following direction:
- Try to parse the path as an EPIC path in the
prepareSCMP()method. - Send back a SCION SCMP response using the SCION path type header, not the EPIC path type header (the SCMP content would be the original EPIC packet however)
- The SCMP type would be a
Parameter Problem, with a new code (e.g., 54) - Add tests to
go/integration/braccept/cases
Can you please let me know if this is the right way to approach this, or point me to a better direction?
go/pkg/router/dataplane.go, line 716 at r13 (raw file):
Previously, shitz (Samuel Hitz) wrote…
I'd slightly prefer
var hvf []byte if scionPath.IsPenultimateHop() { hvf = path.PHVF } else if scionPath.IsLastHop() { hvf = path.LHVF } else { return result, nil } if err := libepic.VerifyHVF(p.cachedMac, &path.PktID, &s, timestamp, hvf); err != nil { return processResult{}, err } return result, nil(should create an SCMP reply of course)
Done. Now with checking the timestamp only at the last two hops, I slightly adapted your suggestions, so the code to check the timestamp is not added redundantly for the penultimate and the last hop.
Regarding the SCMP response, please see my other comment.
go/pkg/router/dataplane.go, line 1256 at r13 (raw file):
Previously, shitz (Samuel Hitz) wrote…
6path.MacLen here and below
Done.
There was a problem hiding this comment.
Reviewed 1 of 11 files at r13, 9 of 9 files at r14.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @mawyss, @oncilla, and @sgmonroy)
go/lib/epic/epic.go, line 34 at r14 (raw file):
in microseconds
drop "in microsecond" here and below
go/lib/epic/epic.go, line 49 at r14 (raw file):
"input", input, "now", now) } diff := now.Sub(input).Microseconds()
With my suggestion below this block can become
diff := now.Sub(input)/TimestampResolution - 1
if diff < 0 {
diff = 0
}
if diff >= (1 << 32) {
return 0, serrors.New("diff between input and now >1d63min", "diff", diff)
}
return uint32(diff), nil
go/lib/epic/epic.go, line 56 at r14 (raw file):
if tsRel >= (1 << 32) { return 0, serrors.New("diff between input and now >1d63min", "diff", diff)
This can now be one line.
go/lib/epic/epic.go, line 66 at r14 (raw file):
packetTimestamp
let's call this epicTimestamp
go/lib/epic/epic.go, line 67 at r14 (raw file):
21
Introduce a constant for this, e.g., `TimestampResolution = 21 * time.Microsecond
go/lib/epic/epic.go, line 200 at r14 (raw file):
} func max(x, y uint64) uint64 {
This is not used anymore.
go/lib/slayers/path/epic/epic_test.go, line 71 at r14 (raw file):
) func TestSerialize(t *testing.T) {
Add tests for error conditions in SerializeTo (at least for a HVF that is not long enough and a nil scion path)
go/lib/slayers/path/epic/epic_test.go, line 249 at r14 (raw file):
PktID: epic.PktID{ Timestamp: 1, Counter: libepic.PktCounterFromCore(2, 3),
These test shouldn't use the helper methods, since PktID.Counter is agnostic to them.
PktCounterFromCore and CoreFromPktCounter should be tested in go/lib/epic/epic_test.go
go/pkg/router/dataplane.go, line 703 at r12 (raw file):
Previously, mawyss wrote…
I return
processResult{}because I want to be sure the packet is dropped, I planned to implement SCMP responses in a separate PR. Please see my other comment regarding SCMP replies.
Ok no problem. Add TODOs then to add SCMP replies in the appropriate places.
go/pkg/router/dataplane.go, line 709 at r12 (raw file):
Previously, mawyss wrote…
If I understand correctly, the border router can currently only return SCMP packets for SCION path type packets, not for arbitrary path types:
prepareSCMP()(dataplane.go) takes the path header aspath := s.scionL.Path.(*scion.Raw).
To implement EPIC SCMP replies I would go in the following direction:
- Try to parse the path as an EPIC path in the
prepareSCMP()method.- Send back a SCION SCMP response using the SCION path type header, not the EPIC path type header (the SCMP content would be the original EPIC packet however)
- The SCMP type would be a
Parameter Problem, with a new code (e.g., 54)- Add tests to
go/integration/braccept/cases
Can you please let me know if this is the right way to approach this, or point me to a better direction?
Overall, that sounds ok to me. Generally, I'm not a big fan of mingling the EPIC handling code too much with the SCION path handling code, however, EPIC-HP is so close to SCION Path handling that it's ok in this case. For other path types we should definitely refactor the code and have different "processors" per path type.
Regarding new SCMP codes: I'm not sure we need to define new codes or if we should just map them to Path Expired (52) and Invalid Hop Field MAC (51) respectively. Paging @oncilla and @sgmonroy to weigh in on this as well.
For this PR, it's fine if you leave out SCMP replies. As discussed above, you should add the appropriate todos.
go/pkg/router/dataplane.go, line 714 at r14 (raw file):
packetTimestamp
packetTimestamp and now can be rolled into the function call directly.
timestamp := time.Unix(int64(info.Timestamp), 0)
if err = libepic.VerifyTimestamp(timestamp, path.PktID.Timestamp, time.Now()); err != nil {
return processResult{}, err
}
go/pkg/router/dataplane_test.go, line 1070 at r14 (raw file):
assertFunc: assert.Error, }, "epic inbound": {
You should add a few more test regarding the epic processing:
- the happy case (this one you have already)
- a malformed epic path case
- an invalid timestamp case
- an invalid MAC case
ideally those would also test that the appropriate SCMP replies are generated, but I see that the current testing code also doesn't do that.
go/pkg/router/dataplane_test.go, line 1118 at r14 (raw file):
Quoted 7 lines of code…
var tsRel uint32 if afterProcessing { tsRel = cachedEpicTsRel } else { tsRel, _ = libepic.CreateTimestamp(time.Unix(int64(timestamp), 0), time.Now()) cachedEpicTsRel = tsRel }
This can be
tsRel := cachedEpicTsRel
if !afterProcessing {
tsRel, err = libepic.CreateTimestamp(...)
require.NoError(t, err)
cachedEpicTsRel = tsRel
}
go/pkg/router/dataplane_test.go, line 1165 at r14 (raw file):
dp := tc.prepareDP(ctrl) input := tc.mockMsg(false) want := tc.mockMsg(true)
why this change?
go/lib/epic/epic_test.go, line 30 at r14 (raw file):
) func TestMacInputGeneration(t *testing.T) {
Test naming uses the pattern Test<name-of-function-under-test>. Here this would be TestPrepareMacInput
go/lib/epic/epic_test.go, line 36 at r14 (raw file):
testCases := map[string]struct { ScionHeader *slayers.SCION Valid bool
Instead of Valid we usually specify the assertion function, i.e., assert.NoError or assert.Error and then in case there is no error you can also compare the results. (same for other tests)
go/lib/epic/epic_test.go, line 72 at r14 (raw file):
} func TestTsRel(t *testing.T) {
TestCreateTimestamp
go/lib/epic/epic_test.go, line 85 at r14 (raw file):
}, "Timestamp more than one day in the past": { Timestamp: now.Add(-30 * 60 * 60 * time.Second),
This should test for edge cases, i.e., 1d63mins
go/lib/epic/epic_test.go, line 89 at r14 (raw file):
}, "Timestamp one day in the past": { Timestamp: now.Add(-24 * 60 * 60 * time.Second),
this should test for the edge case of the maximally allowed timestamp in the past (or add an additional test case for that)
go/lib/epic/epic_test.go, line 91 at r14 (raw file):
4114285713
Can you express these in terms of now and Timestamp or use test cases with simpler outcomes? It looks like you used the function itself to create the results you want to verify it against.
go/lib/epic/epic_test.go, line 93 at r14 (raw file):
Expected: 4114285713, }, "Timestamp one minute in the past": {
These one minute and one second in the past seem arbitrary. You should test for edge cases, i.e., max in the past that is allowed, min in the past that is not allowed, a case where input and now are within 21 microseconds etc.
go/lib/epic/epic_test.go, line 124 at r14 (raw file):
} func TestTimestampVerification(t *testing.T) {
TestVerifyTimestamp
go/lib/epic/epic_test.go, line 134 at r14 (raw file):
// check different cases. tsRel, err := libepic.CreateTimestamp(timeInfoCreation, now) assert.NoError(t, err)
this is not a test thus you should use require.NoError(t, err)
go/lib/epic/epic_test.go, line 190 at r14 (raw file):
} func TestHVFVerification(t *testing.T) {
TestVerifyHVF
There was a problem hiding this comment.
Reviewable status: 9 of 67 files reviewed, 24 unresolved discussions (waiting on @oncilla, @sgmonroy, and @shitz)
go/lib/epic/epic.go, line 34 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
in microsecondsdrop "in microsecond" here and below
Done.
go/lib/epic/epic.go, line 49 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
With my suggestion below this block can become
diff := now.Sub(input)/TimestampResolution - 1 if diff < 0 { diff = 0 } if diff >= (1 << 32) { return 0, serrors.New("diff between input and now >1d63min", "diff", diff) } return uint32(diff), nil
Done, but I replaced diff by tsRel, because with your suggestion diff would not denote a time difference anymore, but the (relative) epic timestamp.
go/lib/epic/epic.go, line 56 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This can now be one line.
Done.
go/lib/epic/epic.go, line 66 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
packetTimestamplet's call this
epicTimestamp
Done.
go/lib/epic/epic.go, line 67 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
21Introduce a constant for this, e.g., `TimestampResolution = 21 * time.Microsecond
Done.
go/lib/epic/epic.go, line 200 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This is not used anymore.
Done.
go/lib/slayers/path/epic/epic_test.go, line 71 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Add tests for error conditions in
SerializeTo(at least for a HVF that is not long enough and a nil scion path)
Done.
go/lib/slayers/path/epic/epic_test.go, line 249 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
These test shouldn't use the helper methods, since
PktID.Counteris agnostic to them.
PktCounterFromCoreandCoreFromPktCountershould be tested ingo/lib/epic/epic_test.go
Done. I specified the Counter as hexadecimal value so that it is easier to understand, and added the PktCounterFromCore and CoreFromPktCounter function following your suggestion.
go/pkg/router/dataplane.go, line 703 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Ok no problem. Add TODOs then to add SCMP replies in the appropriate places.
Done.
go/pkg/router/dataplane.go, line 709 at r12 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Overall, that sounds ok to me. Generally, I'm not a big fan of mingling the EPIC handling code too much with the SCION path handling code, however, EPIC-HP is so close to SCION Path handling that it's ok in this case. For other path types we should definitely refactor the code and have different "processors" per path type.
Regarding new SCMP codes: I'm not sure we need to define new codes or if we should just map them to
Path Expired (52)andInvalid Hop Field MAC (51)respectively. Paging @oncilla and @sgmonroy to weigh in on this as well.For this PR, it's fine if you leave out SCMP replies. As discussed above, you should add the appropriate todos.
I added TODOs to the SCION path type processing result, the timestamp verification and the PHVF/LHVF validation. Furthermore, I implemented a validation of the correct path type in prepareSCMP.
go/pkg/router/dataplane.go, line 714 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
packetTimestamppacketTimestamp and now can be rolled into the function call directly.
timestamp := time.Unix(int64(info.Timestamp), 0) if err = libepic.VerifyTimestamp(timestamp, path.PktID.Timestamp, time.Now()); err != nil { return processResult{}, err }
Done.
go/pkg/router/dataplane_test.go, line 1070 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
You should add a few more test regarding the epic processing:
- the happy case (this one you have already)
- a malformed epic path case
- an invalid timestamp case
- an invalid MAC case
ideally those would also test that the appropriate SCMP replies are generated, but I see that the current testing code also doesn't do that.
Done. I also wrote some auxiliary functions to get rid of redundant code.
go/pkg/router/dataplane_test.go, line 1118 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
var tsRel uint32 if afterProcessing { tsRel = cachedEpicTsRel } else { tsRel, _ = libepic.CreateTimestamp(time.Unix(int64(timestamp), 0), time.Now()) cachedEpicTsRel = tsRel }This can be
tsRel := cachedEpicTsRel if !afterProcessing { tsRel, err = libepic.CreateTimestamp(...) require.NoError(t, err) cachedEpicTsRel = tsRel }
Done.
go/pkg/router/dataplane_test.go, line 1165 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
why this change?
I wanted to make it obvious that tc.mockMsg(false) is executed before tc.mockMsg(true), because the evaluation order matters for the cachedEpicTsRel. I reverted this by now.
go/lib/epic/epic_test.go, line 30 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Test naming uses the pattern
Test<name-of-function-under-test>. Here this would beTestPrepareMacInput
Done.
go/lib/epic/epic_test.go, line 36 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Instead of
Validwe usually specify the assertion function, i.e.,assert.NoErrororassert.Errorand then in case there is no error you can also compare the results. (same for other tests)
Done.
go/lib/epic/epic_test.go, line 72 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
TestCreateTimestamp
Done.
go/lib/epic/epic_test.go, line 85 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
This should test for edge cases, i.e., 1d63mins
Done.
go/lib/epic/epic_test.go, line 89 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
this should test for the edge case of the maximally allowed timestamp in the past (or add an additional test case for that)
Done.
go/lib/epic/epic_test.go, line 91 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
4114285713Can you express these in terms of
nowandTimestampor use test cases with simpler outcomes? It looks like you used the function itself to create the results you want to verify it against.
Done. I manually calculated the value I would expect separately using a calculator. But I agree that having this single value is not very readable / understandable. I replaced it by a calculation.
go/lib/epic/epic_test.go, line 93 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
These one minute and one second in the past seem arbitrary. You should test for edge cases, i.e., max in the past that is allowed, min in the past that is not allowed, a case where input and now are within 21 microseconds etc.
Done. I added tests for the edge cases, but kept the 1 min / 1 sec test cases as they represent the common scenario.
go/lib/epic/epic_test.go, line 124 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
TestVerifyTimestamp
Done.
go/lib/epic/epic_test.go, line 134 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
this is not a test thus you should use
require.NoError(t, err)
Done.
go/lib/epic/epic_test.go, line 190 at r14 (raw file):
Previously, shitz (Samuel Hitz) wrote…
TestVerifyHVF
Done.
There was a problem hiding this comment.
Reviewed 8 of 8 files at r15.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mawyss and @oncilla)
go/cs/onehop/sender_test.go, line 59 at r15 (raw file):
assert.Equal(t, uint16(12), hop.ConsEgress) assert.Equal(t, uint8(63), hop.ExpTime) fullMac := libpath.FullMAC(createMac(t), &info, &hop)
nit - this can call libpath.MAC
go/lib/epic/epic.go, line 42 at r15 (raw file):
tsRel
What do you think about renaming all tsRel instances with epic timestamp in doc strings and epicTS in code?
go/pkg/router/dataplane.go, line 1424 at r15 (raw file):
reflect.TypeOf(path)
just use s.scionL.Path.Type(). reflect is very inefficient.
go/pkg/router/dataplane_test.go, line 1287 at r15 (raw file):
// Encapsulate in IPv4 dst := &net.IPAddr{IP: net.ParseIP("10.0.100.100").To4()} _ = spkt.SetDstAddr(dst)
require.NoError(t, spkt.SetDstAddr(dst))
go/lib/epic/epic_test.go, line 81 at r15 (raw file):
}, "Timestamp one day and 64 minutes in the past": { Timestamp: now.Add(-createTimeHMS(24, 64, 0)),
Shouldn
There was a problem hiding this comment.
Reviewable status: 12 of 18 files reviewed, 4 unresolved discussions (waiting on @karampok, @oncilla, and @shitz)
go/cs/onehop/sender_test.go, line 59 at r15 (raw file):
Previously, shitz (Samuel Hitz) wrote…
nit - this can call
libpath.MAC
Done.
go/lib/epic/epic.go, line 42 at r15 (raw file):
Previously, shitz (Samuel Hitz) wrote…
tsRelWhat do you think about renaming all
tsRelinstances withepic timestampin doc strings andepicTSin code?
Done. I also applied this to the header specification (scion-header.rst).
go/pkg/router/dataplane.go, line 1424 at r15 (raw file):
Previously, shitz (Samuel Hitz) wrote…
reflect.TypeOf(path)just use
s.scionL.Path.Type(). reflect is very inefficient.
Done. Thank you for the hint, I was not aware that reflect does not perform well.
go/pkg/router/dataplane_test.go, line 1287 at r15 (raw file):
Previously, shitz (Samuel Hitz) wrote…
require.NoError(t, spkt.SetDstAddr(dst))
Done.
go/lib/epic/epic_test.go, line 81 at r15 (raw file):
Previously, shitz (Samuel Hitz) wrote…
Shouldn
I don't understand this comment, it looks like the end of the sentence is missing?
There was a problem hiding this comment.
Reviewed 2 of 11 files at r13, 1 of 9 files at r14, 4 of 8 files at r15, 6 of 6 files at r16.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @oncilla and @shitz)
There was a problem hiding this comment.
Reviewed 1 of 5 files at r9.
Reviewable status:
go/lib/epic/epic_test.go, line 81 at r15 (raw file):
Previously, mawyss wrote…
I don't understand this comment, it looks like the end of the sentence is missing?
Sorry that was an unfinished thought that I abandoned, but somehow reviewable still picked it up.
|
/rebase |
github-actions
bot
force-pushed
the
epic4
branch
from
eb99c80
to
00b51b4
Compare
This PR implements the EPIC dataplane as described in [1] and [2]. - Define EPIC packet structure. - Add dataplane handling of EPIC packets. - Add unit tests for packet parsing, EPIC functionality, and the dataplane. Changes to existing code: - mac.go: add function FullMAC(), which returns the full 16 bytes (instead of only 6 bytes) of the MAC. - dataplane.go: add processEPIC() function. - dataplane.go: add "cachedMac" field to the scionPacketProcessor, which allows to cache the (full 16 bytes of the) hop field MAC, such that it does not need to be recalculated by EPIC. Todo for later: - [x] Replace CMAC with CBC-MAC in PHVF/LHVF verification. - [ ] Send back SCMP messages when PHVF/LHVF verification fails (at the moment the packet is just dropped). [1] https://scion.docs.anapaya.net/en/latest/EPIC.html#data-plane [2] https://scion.docs.anapaya.net/en/latest/protocols/scion-header.html#path-type-epic-hp Closes scionproto#3951 GitOrigin-RevId: 7a2cb0a46217b888fc2ee72f48fe9f656d7de5ef
This PR implements the EPIC dataplane as described in [1] and [2]. - Define EPIC packet structure. - Add dataplane handling of EPIC packets. - Add unit tests for packet parsing, EPIC functionality, and the dataplane. Changes to existing code: - mac.go: add function FullMAC(), which returns the full 16 bytes (instead of only 6 bytes) of the MAC. - dataplane.go: add processEPIC() function. - dataplane.go: add "cachedMac" field to the scionPacketProcessor, which allows to cache the (full 16 bytes of the) hop field MAC, such that it does not need to be recalculated by EPIC. Todo for later: - [x] Replace CMAC with CBC-MAC in PHVF/LHVF verification. - [ ] Send back SCMP messages when PHVF/LHVF verification fails (at the moment the packet is just dropped). [1] https://scion.docs.anapaya.net/en/latest/EPIC.html#data-plane [2] https://scion.docs.anapaya.net/en/latest/protocols/scion-header.html#path-type-epic-hp Closes scionproto#3951 GitOrigin-RevId: 7a2cb0a46217b888fc2ee72f48fe9f656d7de5ef
* scion-pki: send CMS signed renewal request
Add the functionality to send CMS signed renewal requests to scion-pki.
By default, the renewal request contains both the CMS signed, and the
legacy signed request.
Either of the requests can be disabled with a feature flag.
GitOrigin-RevId: b0b073d5559b0cde6f9f3b5b5ee2bf6aac5c72ab
* fix minor issue with former rawbytes field
* ca: add delegating handler
Add a delegating handler that is capable of talking to the CA service
for certificate renewal.
This will be plugged in to `renewal/grpc:RenewalServer` based on a config
option in the control service in a follow-up PR.
Currently, the metrics result label values are not set properly
because there is ongoing work in refactoring the metrics for the
renewal package. This will be amended in a follow-up PR.
GitOrigin-RevId: 49ffd3a6d51a06320a927c4dccf50405917ea923
* AT: collect coredumps
If a process crashes inside a container, the coredump is stored within the CI artifact, in the logs directory. The name of the file is container-name.coredump
GitOrigin-RevId: 5578056189dbd866d3e53f6d881c48a8300f3081
* ping: add --healthy-only flag
Add `--healthy-only` flag to `scion ping`. When the --healthy-only option is set, ping first determines healthy paths through probing and chooses amongst them.
GitOrigin-RevId: 954892aeafa993df8614285cad53cb01ff822fb3
* oai: ‘/certificates/{chain-id}’ and ‘/certificates/{chain-id}/blob'
Add the ‘/certificates/{chain-id}’ and ‘/certificates/{chain-id}/blob’ endpoints which provide the certificate chain (blob) for a given chain-id. In addition to the modification of the spec files, the ‘TrsutAPI’ interface is extended to include a function called ‘Chain’.
GitOrigin-RevId: 6c6c7bc3efe50ab51d59e6f3c2cb5f65720446d6
* scion.sh: use 'make lint' instead of './scion.sh lint'
Let us use 'make lint' instead of ‘./scion.sh lint’.
GitOrigin-RevId: 6dca0805e2954edabf9ad02e4735540fd9749a93
* fake-gateway: add dataplane path configuration
Add `forwarding_path` key s.t. the fake-gatway can consume a provided base64 encoded raw dataplane path.
GitOrigin-RevId: f9c1a4657456464c4b928c0f6f3dda763ed5f369
* epic-hp: controlplane
This PR implements the EPIC control plane changes as described in [1]. It improves the structure of the previous PR [2] according to the suggestion from [3].
- Add an EPICDetachedExtension to the beacon. This extension does not get signed and can therefore be detached.
- Add a DigestExtension to the PathSegmentExtensions, which holds the hash of EPICDetachedExtension. This hash is therefore signed.
- Overhead: no new MAC needs to be calculated, only one additional hash per AS.
[1] https://scion.docs.anapaya.net/en/latest/EPIC.html#control-plane
[2] scionproto#3944
[3] scionproto@3a5ea8e
Closes scionproto#3954
GitOrigin-RevId: 44aea772d6021366b7dc8da5de8d7cc1a7b1ad61
* trust: check queries before passing to database
In scionproto/scion/commit/0ebd1f3690d65247cad9b1ac28f016d83e039b1b,
the semantics of `trust.DB.Chains` was slightly modified to allow
fetching certificate chains for the management API.
This change ensures that the verifier always requests certificate chains
with a fully qualified certificate chain query.
GitOrigin-RevId: 1f99f7c8eacaf9c7db46732216c47e270401e804
* prober: use local endpoint per different underlay network
The path prober is now using a local endpoint per different underlay network. Previously, it was using a single static local endpoint which is not correct in cases where different underlay networks are used to reach different next-hop routers.
GitOrigin-RevId: fd962b0fc24b820216bef9664a2f2c3dfc93ebfc
* ca: reduce duplicated code
Define the `ExtractChain` function in `go/pkg/ca/renewal` and reuse it in the gRPC handler implementation.
GitOrigin-RevId: 5d8d5aa46279e0ebaf87ed115822359d46ea5a2d
* ca: refactor ChainRenewal handler
Refactor metrics.
GitOrigin-RevId: 3f64145d6fb9c84b54da12fe8245f5a322d3cb4b
* scrypto: drop unmaintained code
Drop unused and unmaintained code from the `go/lib/scrypto` package.
GitOrigin-RevId: f14fc377dd08cebb47be588ebc137dcb676039d1
* launcher: expose SCION build information in metrics
Expose the SCION build information in the prometheus metrics.
A constant gauge with the `version` label is added to the metrics of
every SCION infrastructure element.
```
scion_build_info{version="v0.6.0"} = 1
```
We might add additional information (e.g., commit ID) in the future
as label values.
GitOrigin-RevId: f7c6b39924fcaa7cbb4a7e353329657db20cabf1
* gateway: fix panic when routing policy is not set
GitOrigin-RevId: 04d8550f4a11c0911fa4b72bbd431844d8f54ed0
* ca: Add JWT authorization support
This adds library support for:
- HTTP clients to add a JWT Bearer token to requests
- HTTP servers to perform authorization before running an HTTP handler
The only allowed algorithm is HS256. Servers will make a strict check
and error out if any other algorithm is used.
For security reasons, keys shorted than 256 bits are also not allowed
(see RFC 7518, Section 3.2).
GitOrigin-RevId: 4553063a4dc6a4e4c378e11414ff74c1add79ef7
* gateway: stop monitoring expired paths
Expired paths were not dropped if they could not be updated with fresh paths. This commit fixes that.
GitOrigin-RevId: 3bcd5d7559f9bf09b6e90defeeef43b3b2d8c43d
* TRC: pem encoding of TRC files
All SCION services and tools are now able to handle raw DER and PEM encoded TRCs.
This is achieved by trying to decode PEM of type `TRC`. If that fails, the TRC is treated as raw DER encoded.
GitOrigin-RevId: 79bb7d003a097d073b41330ad906bde8490120a0
* AT: don't send probes when refreshing caches
GitOrigin-RevId: 51ef82da22ec164ec92f8aa979306c78ec11baeb
* AT: support for tshark tracing in CI
GitOrigin-RevId: 1ff08907164e3eedf72b3813bc36337a769771c1
* AT: use standard package installation for tester images
We've used unnecessary complex distroless-style package installation
for tester images. This patch replaces it with the standard Linux
package installation mechanism.
GitOrigin-RevId: 736a3e1c1282226cd52f6eada2203636bb08642c
* renewal: refactor request verifier
Refactor the `RequestVerifier` to make it more reusable.
Furthermore reduce code duplication and improve error messages.
GitOrigin-RevId: cef2b7b57e919648a8c85f1010df9b861a97f67d
* xtest: pem encoding of TRC
Make function LoadTRC accept pem encoded TRC.
GitOrigin-RevId: da69a433dec63048df97ab08a135d2efd6ab2ec6
* bazel-remote: use v1.3.1 version
bazel-remote released a new 2.0.0 version which lead to some issues in
our CI with S3 backed storage.
Therefore we roll back to v1.3.1 for now.
GitOrigin-RevId: cf67088e2f01b00753a26a7e3556e134f044de5f
* ca: add toggles for legacy and cms handlers
Add new toggle to the control service config in the `[ca]` section:
- `disable_legacy_request`: allows disabling the legacy renewal handler.
- `mode`: allows switching between in-process and delegated certificate
renewal mode.
GitOrigin-RevId: 4035572e1bd173c8ee5ae6d4c4f8266de35628b1
* doc: add phases for sensitive TRC update
Add the phases documentation for the sensitive TRC update ceremony.
The sensitive ceremony is executed in `trc_ceremony_sensitive.sh` and is
part of the CI pipeline.
Some additional info is added to the TRC ceremony preparations document
to prevent operators from overwriting existing keys.
GitOrigin-RevId: c1a9f64c714fd5897278942433af3f95b5840784
* fake-gateway: generate fwd path
Support to generate fresh paths provided each hop key.
GitOrigin-RevId: 476490709db72af4e1a0cf0f7b0d57bdfb4bc5e5
* build: use latest master commit of gomock
Previously we used an out of tree commit, which has now been merged.
GitOrigin-RevId: 937717ea61f0c8b293ac841a5e1fd75e135a2e43
* doc: end host bootstrap design proposal
[doc]
Co-authored-by: FR4NK-W <wirzf@inf.ethz.ch>
Closes scionproto#3943
GitOrigin-RevId: b37bfaedd961a870ed1996363c84912ac7095657
* cs: expose registered CA handlers with metrics
renewal_registered_handlers{type="legacy"} 1
renewal_registered_handlers{type="in-process"} 1
renewal_registered_handlers{type="delegating"} 0
Means the legacy and CMS in-process handlers are registered
GitOrigin-RevId: aab74293c369e3f2578f2fc4e5e830dc868507b6
* crypto: add support for storing symmetric keys in PEM format
This PR adds a few utility functions for dealing with symmetric keys
encoded in PEM format.
GitOrigin-RevId: a61d23dd060c59c4c4183d760205c22cd6c32af0
* daemon/snet/showpaths: allow zero for hop latency
Allow to use zero for the latency metadata of each hop. Zero can be a sensible value for this; for example when multiple interfaces are part of the same router instance, then it makes sense to announce zero for the latency between these interfaces.
Use negative value (-1 nanosecond) to represent undefined values internally, instead.
Closes scionproto#4005
GitOrigin-RevId: 51357848a53f5ff7c196c1b426f93084ece3f2cf
* AT: remove curl from tester containers
GitOrigin-RevId: 443586302f3c646ae285453a6d39ed3d8c61cf05
* bazel-remote: use v2.0.1
After having identified and fixed the issue with the 2.0 release,
this is the 2.0 release that should work for us.
GitOrigin-RevId: e1c28de0f15535fa7b0d441fac8dc803b168d33c
* epic-hp: dataplane
This PR implements the EPIC dataplane as described in [1] and [2].
- Define EPIC packet structure.
- Add dataplane handling of EPIC packets.
- Add unit tests for packet parsing, EPIC functionality, and the dataplane.
Changes to existing code:
- mac.go: add function FullMAC(), which returns the full 16 bytes (instead of only 6 bytes) of the MAC.
- dataplane.go: add processEPIC() function.
- dataplane.go: add "cachedMac" field to the scionPacketProcessor, which allows to cache the (full 16 bytes of the) hop field MAC, such that it does not need to be recalculated by EPIC.
Todo for later:
- [x] Replace CMAC with CBC-MAC in PHVF/LHVF verification.
- [ ] Send back SCMP messages when PHVF/LHVF verification fails (at the moment the packet is just dropped).
[1] https://scion.docs.anapaya.net/en/latest/EPIC.html#data-plane
[2] https://scion.docs.anapaya.net/en/latest/protocols/scion-header.html#path-type-epic-hp
Closes scionproto#3951
GitOrigin-RevId: 7a2cb0a46217b888fc2ee72f48fe9f656d7de5ef
* ca: add support for certificate renewal delegation
Also add support for keeping in-memory objects synchronized with
disk file contents.
GitOrigin-RevId: ae3c9a3bc8d0e50bf92694e5ef753d3f645d39a8
* epic-dp: fix unit test flakyness
The cachedEpicTS variable was accessed by multiple tests in parallel
which lead to flakyness. Instead of the caching approach fix the time in
the beginning of the test and inject it.
GitOrigin-RevId: e185b32b4378691bf4411b58acc0de1e855172c2
Co-authored-by: Dominik Roos <roos@anapaya.net>
Co-authored-by: Martin Sustrik <sustrik@anapaya.net>
Co-authored-by: Samuel Hitz <hitz@anapaya.net>
Co-authored-by: noorizea <60094353+noorizea@users.noreply.github.com>
Co-authored-by: gbrel <cristache@anapaya.net>
Co-authored-by: mawyss <mawyss@ethz.ch>
Co-authored-by: Konstantinos <karampogias@anapaya.net>
Co-authored-by: Sergiu Costea <sergiu.costea@gmail.com>
Co-authored-by: Lukas Vogel <vogel@anapaya.net>
Co-authored-by: Sergio Gonzalez Monroy <monroy@anapaya.net>
Co-authored-by: Andrea Tulimiero <tulimiero.andrea@gmail.com>
Co-authored-by: FR4NK-W <wirzf@inf.ethz.ch>
Co-authored-by: Matthias Frei <matthias.frei@inf.ethz.ch>
This PR implements the EPIC dataplane as described in [1] and [2].
Changes to existing code:
Todo for later:
[1] https://scion.docs.anapaya.net/en/latest/EPIC.html#data-plane
[2] https://scion.docs.anapaya.net/en/latest/protocols/scion-header.html#path-type-epic-hp
This change is