Re: [PATCH ulogd] log NAT events using IPFIX

[Tomasz Pala <gotar@polanet.pl>]

Hi,

On Tue, Dec 12, 2023 at 14:47:25 +0100, Pablo Neira Ayuso wrote:

> Would you mind to split this large patch is smaller chunks, logic is:
> 
> - one logical update per patch
> 
> with a description on the rationale, probably in 3-4 patches?

Sure.

>>  /* Information Element Identifiers as of draft-ietf-ipfix-info-11.txt */
>> +/* https://www.iana.org/assignments/ipfix/ipfix.xhtml */
>>  enum {
>>  	IPFIX_octetDeltaCount		= 1,
>>  	IPFIX_packetDeltaCount		= 2,
>> -	/* reserved */
>> +	/* deltaFlowCount */
>>  	IPFIX_protocolIdentifier	= 4,
>>  	IPFIX_classOfServiceIPv4	= 5,
>>  	IPFIX_tcpControlBits		= 6,
>> @@ -73,24 +74,24 @@ enum {
>>  	IPFIX_flowLabelIPv6		= 31,
>>  	IPFIX_icmpTypeCodeIPv4		= 32,
>>  	IPFIX_igmpType			= 33,
>> -	/* reserved */
>> -	/* reserved */
>> +	/* samplingInterval */
>> +	/* samplingAlgorithm */
>>  	IPFIX_flowActiveTimeOut		= 36,
>>  	IPFIX_flowInactiveTimeout	= 37,
>> -	/* reserved */
>> -	/* reserved */
>> +	/* engineType */
>> +	/* engineId */
> 
> These comments updates are to get this in sync with RFC? What is the
> intention?

Well, the list was already there, but based on older document. Link to
current one is added at the top of this chunk:
	/* https://www.iana.org/assignments/ipfix/ipfix.xhtml */

I could have replaced the comments with actual assignments, like:

	IPFIX_engineId			= 41

but I personally don't like creating entities that are not used in the
rest of the code. I personally prefer doing (uncommenting) this on entry-by-entry
basis, when something get's actually implemented.
This approach gives some insight about implementation status.

>> +		Assigned for NetFlow v9 compatibility
>> +		postIpDiffServCodePoint
>> +		plicationFactor
>> +		className
>> +		classificationEngineId
>> +		layer2packetSectionOffset
>> +		layer2packetSectionSize
>> +		layer2packetSectionData
>> +	105-127	Assigned for NetFlow v9 compatibility	*/
>>  	IPFIX_bgpNextAdjacentAsNumber	= 128,
>>  	IPFIX_bgpPrevAdjacentAsNumber	= 129,
>>  	IPFIX_exporterIPv4Address	= 130,

There the rationale was - up to this point we got _all_ the assignments
(continuously). But as we go further, there is more and more data types
that won't be ever implemented.

Actually the question should be: what was the purpose of this listing?

https://git.netfilter.org/ulogd2/commit/include/ulogd/ipfix_protocol.h?id=570f2229563fb8101b1ba0369eeda1f19dbc88ee

Anyway - this chunk is actually redundant (except from the source
document), so I think I'll simply drop this. No need for copy&paste IANA.

>> +++ b/input/flow/ulogd_inpflow_NFCT.c
>> @@ -379,10 +379,10 @@ static struct ulogd_key nfct_okeys[] = {
>>  		.type 	= ULOGD_RET_UINT32,
>>  		.flags 	= ULOGD_RETF_NONE,
>>  		.name	= "flow.start.usec",
>> -		.ipfix	= {
>> +	/*	.ipfix	= {
>>  			.vendor		= IPFIX_VENDOR_IETF,
>> -			.field_id	= IPFIX_flowStartMicroSeconds,
>> -		},
>> +			.field_id	= IPFIX_flowStartMicroSeconds,	-- this entry expects absolute total value, not the subsecond remainder
>> +		},	*/
>>  	},
>>  	{
>>  		.type	= ULOGD_RET_UINT32,
>> @@ -397,10 +397,10 @@ static struct ulogd_key nfct_okeys[] = {
>>  		.type	= ULOGD_RET_UINT32,
>>  		.flags	= ULOGD_RETF_NONE,
>>  		.name	= "flow.end.usec",
>> -		.ipfix	= {
>> +	/*	.ipfix	= {
>>  			.vendor		= IPFIX_VENDOR_IETF,
>> -			.field_id	= IPFIX_flowEndSeconds,
>> -		},
>> +			.field_id	= IPFIX_flowEndMicroSeconds,	-- this entry expects absolute total value, not the subsecond remainder
>> +		},	*/
> 
> This is commented out, if it needs to go, better place this in a patch
> and explain why you propose this change?

These are disabled, because they're simply not correct. Left in place as a
comment just to left a warning for future updates.

https://git.netfilter.org/ulogd2/commit/input/flow/ulogd_inpflow_NFCT.c?id=6b4b8aa3a9612f1c92e885ac71a503321cabd69e

I might as well simply change it to ".ipfix = { }".

-- 
Tomasz Pala <gotar@pld-linux.org>