Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TEST] Add state machine #63

Merged
merged 12 commits into from
Jan 31, 2024
Merged

[TEST] Add state machine #63

merged 12 commits into from
Jan 31, 2024

Conversation

Raguideau
Copy link
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have read the CONTRIBUTING document.
  • Each of my commits messages include Signed-off-by: Author Name <[email protected]> to accept the DCO.

@Raguideau Raguideau requested a review from linouxis9 as a code owner January 8, 2024 15:06
Copy link
Member

@linouxis9 linouxis9 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your PR!

A suggestion: if we're going to do things properly, we should do them this way:

  • Instead of having the stateMachine in a hook, it should become an integral part of the aio5gc's UEState, which would make it possible to properly implement the transition
    RegistrationRequest -> AuthReq and
    RegistrationRequest -> RegistrationAcccept depending on current status, for example.
    And if AuthReject, we start again from the beginning etc... basically allowing us to validate the whole call flow.
  • And then, we modify the hook system so that we can insert a hook at each transition (eg. in the callbacks) rather than as we do at present, as the whole point of having a stateMachine with transitions was to check the correct flow of messages, which might be different in each test.

Make sure to accept DCO as well.

@linouxis9
Copy link
Member

linouxis9 commented Jan 9, 2024

Thanks for the new commit!
It's not exactly what we should do.
The state machine should be intrinsic to the aio5gc, it should not be set by the tests.
The whole point of having a state machine in aio5gc is that you don't have to do things like this:

	if !ue.GetInitialContextSetup() {
		return errors.New("[5GC][NGAP] This UE has no security context set up")
	}
	if ue != ranUe {
		return errors.New("[5GC][NGAP] RanUeNgapId does not match the one registred for this UE")
	}

Basically, the point is that you have one handler per state (eg. the callback).
You don't need to do these kinds of checks:
In the Registered state handler, you can handle a Session Establishment PDU, and a Deregister.
However, it won't be possible in the fresh state handler.
And then we can have hooks in each of these handlers (using a map msg.MsgType -> hook and call it generically from each callbacks.)

Note that you can have several PDU Sessions, so you'll need a global state machine + one per PDU Session (this is what's normally done in the AMF for the 5GMM state machine the SMF for the 5GSM state machine per PDU Session).

This is something we would also need to implement on PacketRusher's side instead of our current big global handler.

See for reference: 24.501 Figure 5.1.3.2.1.1.1: 5GMM main states in the UE:

https://www.tech-invite.com/3m24/toc/tinv-3gpp-24-501_l.html

And 24.501 Figure 6.1.3.2.1.1: The 5GSM sublayer states for PDU session handling in the UE:

https://www.tech-invite.com/3m24/toc/tinv-3gpp-24-501_zg.html

test/aio5gc/build.go Outdated Show resolved Hide resolved
test/aio5gc/context/sessionManagement.go Show resolved Hide resolved
@@ -46,7 +46,8 @@ func Encode(ue *context.UEContext, msg *nas.Message) ([]byte, error) {
}

// Plain NAS message
if ue == nil || !ue.SecurityContextAvailable {

if ue == nil || !(ue.GetSecurityContext() != nil) {
Copy link
Member

Choose a reason for hiding this comment

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

You should use your state machine instead of doing this kind of check. You should have a branch of states where you are sure that securityContext is available. The goal of having a state machine is that when you arrive to a given state, you are sure that some preconditions are valid, here that the SecurityContext is available.
Then you have an Encode for state SecurityContextAvailable and an when it is not the case. Same for Decode.

test/aio5gc/msg/nas/builder/registrationAccept.go Outdated Show resolved Hide resolved
test/aio5gc/msg/nas/handler/authenticationResponse.go Outdated Show resolved Hide resolved
test/aio5gc/msg/nas/nasDispatcher.go Outdated Show resolved Hide resolved
test/aio5gc/msg/nas/nasDispatcher.go Outdated Show resolved Hide resolved
test/aio5gc/msg/ngap/builder/ueContextReleaseCommand.go Outdated Show resolved Hide resolved
test/pr_test.go Outdated Show resolved Hide resolved
test/pr_test.go Show resolved Hide resolved
@linouxis9
Copy link
Member

Thanks a lot for your work!
Make sure to sign DCO for the previous commits as well
See the commands at: https://github.com/HewlettPackard/PacketRusher/pull/63/checks?check_run_id=20501740969

Copy link
Member

@linouxis9 linouxis9 left a comment

Choose a reason for hiding this comment

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

Cool! Make sure that nasEncoder changes depending the states to avoid all these checks a bit everywhere
Same for decoder, you should not allow a non-integrity protected message after auth.
Don't forget about DCO on previous commits, no FSM global, and previous comments as well :-)
Thanks a lot!

test/aio5gc/context/sessionManagement.go Show resolved Hide resolved
test/aio5gc/context/ue.go Outdated Show resolved Hide resolved
test/aio5gc/context/ue.go Outdated Show resolved Hide resolved
* © Copyright 2023 Hewlett Packard Enterprise Development LP
*/
package codec

Copy link
Member

Choose a reason for hiding this comment

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

We have the same code in the core PacketRusher code, could it be factorized?

Copy link
Member

Choose a reason for hiding this comment

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

This comment again

test/aio5gc/msg/nas/handler/authenticationResponse.go Outdated Show resolved Hide resolved
@Raguideau Raguideau force-pushed the test branch 4 times, most recently from 90f9c22 to 103707d Compare January 31, 2024 10:59
test/aio5gc/context/amf.go Outdated Show resolved Hide resolved
test/aio5gc/context/amf.go Outdated Show resolved Hide resolved
test/aio5gc/context/ue.go Outdated Show resolved Hide resolved
test/aio5gc/context/ue.go Outdated Show resolved Hide resolved
* © Copyright 2023 Hewlett Packard Enterprise Development LP
*/
package codec

Copy link
Member

Choose a reason for hiding this comment

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

This comment again

log "github.com/sirupsen/logrus"
)

func Encode(ue *context.UEContext, msg *nas.Message) ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Can it be factorized with PacketRusher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we should also be able to factorize authentication. I will do it in another pr.

@Raguideau Raguideau changed the title Test [TEST] Add state machine Jan 31, 2024
@Raguideau Raguideau merged commit cec07b7 into HewlettPackard:main Jan 31, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants