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

Spec: Store spiffeID of NSE in NetworkSerivceEndpoint message #148

Closed
NikitaSkrynnik opened this issue Oct 14, 2022 · 4 comments
Closed
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@NikitaSkrynnik
Copy link
Contributor

NikitaSkrynnik commented Oct 14, 2022

Description

Current implementation of authorization for registry may be insecure when registry restarts.

Current behavior

When NSE goes to registry to register itself, registry stores it's spiffeID in a map. Registry uses this map to check that different spiffeIDs don't register NSEs with the same names. When registry restarts, the map is removed, and bad NSE has a possibility to register itself under the name of a good NSE.

Solution

  • We can use gRPC metadata to transfer path, which is private and constracted during Registeration of Endpoint. Authorize chain element can get private path from gRPC medatada and check policies.
  • We can store spiffeID in NSE entry in etcd. When a bad NSE tries to register itself as a good NSE, we can get entry from etcd and check that spiffeID of a good one is equal to spiffeID of a bad one. If spiffeIDs are different, registry won't allow bad NSE to register. It would work even if registry restarted, because spiffeID of good NSE is stored in etcd. (Options for how we can store spiffe id are listed below)

Options

We can store additional information in NSE CRD that can be helpful for future authorization cases. Also we can use NSE labels to store spiffeID.

Option 1: Store only NSE spiffeID in NSE CRD

Fixes registry security issue.

message NetworkServiceEndpoint {
    string name = 1;
    repeated string network_service_names = 2;
    map<string, NetworkServiceLabels> network_service_labels = 3;
    string url = 4;
    string id = 5;
    google.protobuf.Timestamp expiration_time = 6;
    google.protobuf.Timestamp initial_registration_time = 7;
}

Option 2: Store a list of spiffeIDs in NSE CRD from the path that was constructed during initial NSE registration

Fixes registry security issue. It may be useful for authorization cases that we haven't considered yet

message NetworkServiceEndpoint {
    string name = 1;
    repeated string network_service_names = 2;
    map<string, NetworkServiceLabels> network_service_labels = 3;
    string url = 4;
    repeated string path_ids = 5;
    google.protobuf.Timestamp expiration_time = 6;
    google.protobuf.Timestamp initial_registration_time = 7;
}

Option 3: Store spiffeID of NSE in NSE labels.

Fixes registry sequrity issue. Doesn't require any changes in API

nse := &registryapi.NetworkServiceEndpoint{
		Name: "nse",
		NetworkServiceLabels: map[string]*registryapi.NetworkServiceLabels{
			"networkservice": {
				Labels: map[string]string{
					"id": "spiffe://test.com/nse",
				},
			},
		},
		NetworkServiceNames: []string{"networkservice"},
	}
@denis-tingaikin
Copy link
Member

denis-tingaikin commented Oct 14, 2022

@NikitaSkrynnik Could you also add proto examples to clarify options?

# TODO

@denis-tingaikin denis-tingaikin changed the title Proposal: Store spiffeID of NSE in NetworkSerivceEndpoint message Spec: Store spiffeID of NSE in NetworkSerivceEndpoint message Oct 27, 2022
@denis-tingaikin denis-tingaikin added the enhancement New feature or request label Oct 27, 2022
@denis-tingaikin denis-tingaikin added this to the v1.7.0 milestone Oct 27, 2022
@denis-tingaikin denis-tingaikin moved this to In Progress in Release v1.7.0 Nov 21, 2022
@denis-tingaikin denis-tingaikin moved this from In Progress to Done in Release v1.7.0 Nov 29, 2022
@NikitaSkrynnik NikitaSkrynnik moved this from Done to In Progress in Release v1.7.0 Nov 30, 2022
@NikitaSkrynnik
Copy link
Contributor Author

NikitaSkrynnik commented Dec 1, 2022

Subtasks

  • Refactor updatepath registry chain element ~ 12h
  • Update all cmd repositories ~ 12h
  • Check and fix integration tests ~ 24h

@denis-tingaikin denis-tingaikin moved this from In Progress to Done in Release v1.7.0 Dec 13, 2022
@denis-tingaikin
Copy link
Member

Seems like completed by #148 (comment)

@NikitaSkrynnik Thanks!

LionelJouin added a commit to Nordix/Meridio that referenced this issue Dec 13, 2022
Authorization has been introduced as part of v1.7.0-rc.1
networkservicemesh/api#148
These changes are then now require to register a NS or NSE.
LionelJouin added a commit to Nordix/Meridio that referenced this issue Dec 16, 2022
Authorization has been introduced as part of v1.7.0.
networkservicemesh/api#148
These changes are then now require to register a NS or NSE if registry
policies are enabled in nsmgr.
LionelJouin added a commit to Nordix/Meridio that referenced this issue Dec 20, 2022
Authorization has been introduced as part of v1.7.0.
networkservicemesh/api#148
These changes are then now require to register a NS or NSE if registry
policies are enabled.
LionelJouin added a commit to Nordix/Meridio that referenced this issue Dec 20, 2022
Authorization has been introduced as part of v1.7.0.
networkservicemesh/api#148
These changes are then now require to register a NS or NSE if registry
policies are enabled.
LionelJouin added a commit to Nordix/Meridio that referenced this issue Dec 20, 2022
Authorization has been introduced as part of v1.7.0.
networkservicemesh/api#148
These changes are then now require to register a NS or NSE if registry
policies are enabled.
LionelJouin added a commit to Nordix/Meridio that referenced this issue Dec 20, 2022
Authorization has been introduced as part of v1.7.0.
networkservicemesh/api#148
These changes are then now require to register a NS or NSE if registry
policies are enabled
uablrek pushed a commit to Nordix/Meridio that referenced this issue Jan 4, 2023
Authorization has been introduced as part of v1.7.0.
networkservicemesh/api#148
These changes are then now require to register a NS or NSE if registry
policies are enabled
LionelJouin added a commit to Nordix/Meridio that referenced this issue Jan 9, 2023
Authorization has been introduced as part of v1.7.0.
networkservicemesh/api#148
These changes are then now require to register a NS or NSE if registry
policies are enabled
LionelJouin added a commit to Nordix/Meridio that referenced this issue Jan 9, 2023
Authorization has been introduced as part of v1.7.0.
networkservicemesh/api#148
These changes are then now require to register a NS or NSE if registry
policies are enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

3 participants