Add security support for exporter and collector #57

zyiou · 2020-10-23T06:21:20Z

Fixes #50

srikartati · 2020-10-24T07:53:18Z

pkg/collector/tcp.go

@@ -40,6 +41,43 @@ func (cp *collectingProcess) startTCPServer() {
 	}
 }

+func (cp *collectingProcess) StartTLSServer(serverCert []byte, serverKey []byte) {


Is TLSserver not supported with UDP transport? I see TLSclient in exporting process written for both TCP and UDP transport.

Just realized you had WIP in the title. If you are planning to address the above already, please ignore the comment.

For UDP Transport, we will need to use DTLS. Still working on that.

StartTLSServer(serverCert []byte, serverKey []byte) can be simplified as StartTLSServer(serverCert, serverKey []byte)

@shihhaoli Thanks! will update it.

codecov · 2020-11-05T06:14:57Z

Codecov Report

Merging #57 (2eb85c8) into master (150f98e) will decrease coverage by 0.88%.
The diff coverage is 68.53%.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   81.14%   80.26%   -0.89%     
==========================================
  Files          13       13              
  Lines        1496     1591      +95     
==========================================
+ Hits         1214     1277      +63     
- Misses        191      212      +21     
- Partials       91      102      +11

Flag	Coverage Δ
integration-tests	`67.07% <64.33%> (-0.14%)`	⬇️
unit-tests	`79.63% <66.43%> (-1.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/collector/tcp.go	`67.74% <57.14%> (-1.83%)`	⬇️
pkg/exporter/process.go	`69.23% <60.52%> (-2.95%)`	⬇️
pkg/collector/udp.go	`67.77% <70.00%> (-0.98%)`	⬇️
pkg/intermediate/aggregate.go	`75.56% <71.42%> (ø)`
pkg/collector/process.go	`91.35% <94.11%> (+0.50%)`	⬆️

srikartati · 2020-11-09T05:18:08Z

pkg/collector/process.go

-func (cp *collectingProcess) Start() {
-	if cp.address.Network() == "tcp" {
-		cp.startTCPServer()
-	} else if cp.address.Network() == "udp" {
-		cp.startUDPServer()
-	}
-}
-


Is this removed to let the user start regular tcp/udp server or server with TLS/DTLS encryption? If so, I feel it is better to take in a input from the user through InitCollectorProcess on whether to use encryption or not.

Just checking if these comments are resolved or not. From the latest code changes, it doesn't seem so.

Thanks for reminding. Changes got postponed by other PRs. Will update asap.

srikartati · 2020-11-09T05:21:12Z

pkg/exporter/process.go

@@ -98,6 +101,60 @@ func InitExportingProcess(collectorAddr net.Addr, obsID uint32, tempRefTimeout u
 	return expProc, nil
 }

+func InitExportingProcessWithTLS(collectorAddr net.Addr, obsID uint32, cert []byte) (*ExportingProcess, error) {


Here as well taking the input for encryption may be better. We can save some duplication of code and make this library function simpler.

srikartati

Should we make the standalone collector take in the certificate and the key as command arguments?

I think this will go in after PR #91 and PR #93, right?

srikartati · 2020-11-30T14:56:58Z

pkg/test/exporter_collector_test.go

 func TestSingleRecordUDPTransport(t *testing.T) {
-	address, err := net.ResolveUDPAddr("udp", "0.0.0.0:4739")
+	address, err := net.ResolveUDPAddr("udp", "0.0.0.0:4630")


let's use random port by giving 0?

Sure. updated with random port, but also added some code to support the random port since previously we directly used hard-coded address passed from users. If you think the commit is not quite relevant to current PR, I can move it to another PR separately.

Since you changed the port number other than 4739 in the tests, I suggested a random open port. We should only expect users to give 4739, since it is the standard IPFIX port, right?

What is the extra code required to accept random port? Is that required in tests code or actual code?

That's true. I changed the port number because if we use same port for all the tests, it will have conflicts (some of the tests cannot start collector since the port is occupied.). extra code is mainly about update address after the listener resolves the random port. I also added a get address function for get the latest address of the collector.

srikartati · 2020-11-30T18:13:45Z

Should we make the standalone collector take in the certificate and the key as command arguments?

I think this will go in after PR #91 and PR #93, right?

We can extend the standalone collector in a different PR. Overall it looks good to me.
@shihhaoli Please review this when you get a chance.

srikartati · 2020-12-01T05:17:04Z

pkg/intermediate/aggregate_test.go

@@ -167,6 +167,7 @@ func TestGetTupleRecordMap(t *testing.T) {
 }

 func TestAggregateMsgByFlowKey(t *testing.T) {
+	registry.LoadRegistry()


Do it in init, so it will be applicable for all test functions.

srikartati · 2020-12-01T08:24:46Z

pkg/test/exporter_collector_test.go

 func TestSingleRecordUDPTransport(t *testing.T) {
-	address, err := net.ResolveUDPAddr("udp", "0.0.0.0:4739")
+	address, err := net.ResolveUDPAddr("udp", "0.0.0.0:4630")


Since you changed the port number other than 4739 in the tests, I suggested a random open port. We should only expect users to give 4739, since it is the standard IPFIX port, right?

What is the extra code required to accept random port? Is that required in tests code or actual code?

shihhaoli · 2020-12-01T17:07:22Z

cmd/collector/collector.go

@@ -130,7 +130,7 @@ func run() error {
 		return fmt.Errorf("input given ipfix.transport flag is not supported or valid")
 	}
 	// Initialize collecting process
-	cp, err := collector.InitCollectingProcess(netAddr, 65535, 0)
+	cp, err := collector.InitCollectingProcess(netAddr, 65535, 0, false, nil, nil)


For public methods, we can consider creating a struct for all the input parameters.
This can avoid changing the code on the caller side whenever we need to add new parameters in the future.
I was asked by WCP team to do something like that when they consume a library maintained by our team. Otherwise every time we upgrade our library, that could break their existing code.

For example, a struct like https://gitreview.eng.vmware.com/c/nsx/+/312057/7/solutions/nsxi/go/src/nsxi/validator/validator.go#61
and used in https://gitreview.eng.vmware.com/c/nsx/+/312057/7/solutions/nsxi/go/src/nsxi/validator/validator.go#311

Thanks it will also be clearer for users to pass parameters. Updated for exporter and intermediate process too.

@shihhaoli It is a good suggestion to make consumption easier.
However, it is better to avoid company-specific internal links in the OSS repo.

Yes, I made the same mistake while developing openstack long time ago (referring bugzilla link for bug fix in the commit message) :-)
Any suggestion how to do this in the future? Send an email instead?

I felt if there are non-vmware folks following the conversation, they may not get complete context with the links that are not accessible. Yes offline mode through email or slack sounds good to me.

OK, then those non-vmware people will completely hide from our conversations, but at least they will not complain inaccessible links : -)

Or keeping the context to an accessible project will also work :)

shihhaoli · 2020-12-01T18:08:10Z

pkg/intermediate/aggregate.go

+			if err != nil {
+				return fmt.Errorf("IANA Registry is not loaded correctly with originalExporterIPv4Address.")
+			}
+			address = net.ParseIP(message.ExportAddress).To4()


You may rearrange the code so that net.ParseIP() can be executed only once (i.e. lines 281 and 286, also lines 287 and 292).

srikartati

Address related extra code looks fine to me.

srikartati · 2020-12-02T03:51:13Z

pkg/intermediate/aggregate.go

+			}
+			address = net.ParseIP(message.ExportAddress).To16()
+		} else {
+			return fmt.Errorf("original exporter address is not in correct format.")


As a convention, punctuation not required in fmt.Errorf.

srikartati

LGTM except for one comment on TODO.

srikartati · 2020-12-04T08:20:36Z

pkg/exporter/process.go

-		klog.Errorf("Cannot the create the connection to configured ExportingProcess %s: %v", collectorAddr.String(), err)
-		return nil, err
-	}
+// TODO: Get obsID, tempRefTimeout as args which can be of dynamic size supporting both TCP and UDP.


this todo not needed anymore right?

yes, removed it. Thanks!

vmwclabot added the cla-not-required label Oct 23, 2020

srikartati reviewed Oct 24, 2020

View reviewed changes

zyiou force-pushed the zyiou/tls branch 2 times, most recently from d130042 to 74abc0c Compare October 27, 2020 19:05

zyiou force-pushed the zyiou/tls branch 3 times, most recently from e5c8be0 to 98ab187 Compare November 5, 2020 01:45

zyiou force-pushed the zyiou/tls branch from 28344d5 to ef49c53 Compare November 8, 2020 08:18

zyiou changed the title ~~[WIP] Add security support for exporter and collector~~ Add security support for exporter and collector Nov 8, 2020

srikartati reviewed Nov 9, 2020

View reviewed changes

zyiou force-pushed the zyiou/tls branch 3 times, most recently from 0186ea8 to 02e0484 Compare November 24, 2020 05:54

srikartati reviewed Nov 30, 2020

View reviewed changes

zyiou force-pushed the zyiou/tls branch from a80ebf0 to 7ea9f81 Compare December 1, 2020 05:10

srikartati reviewed Dec 1, 2020

View reviewed changes

shihhaoli reviewed Dec 1, 2020

View reviewed changes

zyiou force-pushed the zyiou/tls branch from ba5f042 to dd85f7f Compare December 1, 2020 22:23

srikartati reviewed Dec 2, 2020

View reviewed changes

zyiou force-pushed the zyiou/tls branch 4 times, most recently from 24e5d6d to 2a5aad5 Compare December 4, 2020 05:05

srikartati previously approved these changes Dec 4, 2020

View reviewed changes

zyiou dismissed srikartati’s stale review via 6fa758c December 4, 2020 18:11

add tls/dtls support

2eb85c8

zyiou force-pushed the zyiou/tls branch from 6fa758c to 2eb85c8 Compare December 4, 2020 18:12

srikartati approved these changes Dec 4, 2020

View reviewed changes

zyiou merged commit 72176b5 into vmware:master Dec 4, 2020

zyiou added a commit to zyiou/go-ipfix that referenced this pull request Feb 5, 2021

add tls/dtls support (vmware#57)

6f28b32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add security support for exporter and collector #57

Add security support for exporter and collector #57

zyiou commented Oct 23, 2020 •

edited

Loading

srikartati Oct 24, 2020

srikartati Oct 24, 2020

zyiou Oct 25, 2020

shihhaoli Oct 27, 2020

zyiou Oct 27, 2020

codecov bot commented Nov 5, 2020 •

edited

Loading

srikartati Nov 9, 2020

srikartati Nov 23, 2020

zyiou Nov 24, 2020

srikartati Nov 9, 2020

srikartati left a comment

srikartati Nov 30, 2020

zyiou Dec 1, 2020

srikartati Dec 1, 2020

zyiou Dec 1, 2020

srikartati commented Nov 30, 2020

srikartati Dec 1, 2020

srikartati Dec 1, 2020

shihhaoli Dec 1, 2020 •

edited

Loading

zyiou Dec 1, 2020

srikartati Dec 2, 2020 •

edited

Loading

shihhaoli Dec 2, 2020

srikartati Dec 2, 2020

shihhaoli Dec 2, 2020

srikartati Dec 2, 2020

shihhaoli Dec 1, 2020

srikartati left a comment

srikartati Dec 2, 2020

srikartati left a comment

srikartati Dec 4, 2020

zyiou Dec 4, 2020

Add security support for exporter and collector #57

Add security support for exporter and collector #57

Conversation

zyiou commented Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 5, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shihhaoli Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyiou commented Oct 23, 2020 •

edited

Loading

codecov bot commented Nov 5, 2020 •

edited

Loading

shihhaoli Dec 1, 2020 •

edited

Loading

srikartati Dec 2, 2020 •

edited

Loading