Set a user agent for http/https tdnf requests #453

shivania2 · 2023-10-09T15:32:06Z

tdnf does not set a user agent for http/https requests and the repository pkgs.k8s.io did block requests without user agents (because its the default policy in AWS WAF). This issue is fixed in kubernetes upstream repo. But to avoid this kind of issue in future, adding user agent for http/https tdnf requests

vmwclabot · 2023-10-09T15:32:13Z

@shivania2, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

oliverkurth

We also need a test.

client/config.c

oliverkurth · 2023-10-09T18:12:27Z

client/config.c

+
+    while (fgets(line, 2*UserAgentHeaderMaxLength, file)) {
+        if (strstr(line, "ID=") != NULL) {
+            sscanf(line, "ID=\"%[^\"]\"", pConf->pszOSName);


Have you tested that this works with both unquoted and quoted settings?

client/config.c

oliverkurth · 2023-10-09T18:15:06Z

client/prototypes.h

+uint32_t
+TDNFParseOSInfo(


You can make this a static function in config.c.

Can we use config parser for parsing /etc/os-release?

Also, we have some info on OS with uname APIs? can we use that here as well?

Can we use config parser for parsing /etc/os-release?

No.

Also, we have some info on OS with uname APIs? can we use that here as well?

I don't think it's needed.

oliverkurth · 2023-10-09T18:15:56Z

client/config.c

+        strcat(pszUserAgentHeader, " ");
+    }
+    else
+        strcat(pszUserAgentHeader, "UNKNOWN ");


Nit: use "unknown". No need to yell.

client/config.c

sshedi · 2023-10-10T06:07:23Z

client/config.c

@@ -91,6 +139,8 @@ TDNFReadConfig(
        }
    }

+    TDNFParseOSInfo(pConf);


validate return value here

sshedi · 2023-10-10T06:10:40Z

client/config.c

+    dwError = TDNFAllocateMemory(UserAgentHeaderMaxLength, sizeof(char), (void**)&pszUserAgentHeader);
+    BAIL_ON_TDNF_ERROR(dwError);
+
+	strcpy(pszUserAgentHeader, "User-Agent:tdnf/");


"User-Agent:tdnf/" storing these keywords in macros would help

If we use TDNFAllocateStringPrintf() it will be a format string.

sshedi · 2023-10-10T06:12:29Z

client/prototypes.h

+uint32_t
+TDNFParseOSInfo(


Can we use config parser for parsing /etc/os-release?

Also, we have some info on OS with uname APIs? can we use that here as well?

sshedi · 2023-10-10T06:13:24Z

client/config.c

+    strcat(pszUserAgentHeader, "/");
+
+    if (pConf->pszOSVersion != NULL)
+        strcat(pszUserAgentHeader, pConf->pszOSVersion);


use snprintf here.

client/config.c

sshedi · 2023-10-10T06:14:44Z

client/config.c

+    char *line = NULL;
+    uint32_t dwError = 0;
+    FILE *file = NULL;
+    file = fopen("/etc/os-release", "r");


new line before line 39 and store etc/os-release path in macro

sshedi · 2023-10-10T06:15:49Z

client/config.c

+    dwError = TDNFAllocateMemory(2*UserAgentHeaderMaxLength, sizeof(char), (void**)&line);
+    BAIL_ON_TDNF_ERROR(dwError);
+
+    dwError = TDNFAllocateMemory(UserAgentHeaderMaxLength, sizeof(char), (void**)&pConf->pszOSName);


50 bytes for name & version seems like an overkill and allocate memory based on line length inside loop.

shivania2 · 2023-10-10T08:24:00Z

We also need a test.

Exploring the possible test case

sshedi · 2023-10-10T09:20:26Z

client/config.c

+    }
+
+    while (fgets(temp, USERAGENTHEADERMAXLENGTH, file)) {
+        if (strstr(temp, "ID=") != NULL && name_found == 0) {


use strncmp("ID=", temp, sizeof("ID=")-1);

no need of name_found and it will make this logic bit more robust

sshedi · 2023-10-10T09:21:44Z

client/config.c

+        BAIL_ON_TDNF_SYSTEM_ERROR(dwError);
+    }
+
+    while (fgets(temp, USERAGENTHEADERMAXLENGTH, file)) {


Use getline instead. Example here https://stackoverflow.com/questions/3501338/c-read-file-line-by-line

fgets() is just fine.

fgets works here but we assume that max line length is 50 bytes, with getline we don't need not make any assumptions and fits perfectly for this use case.

50 characters is too short, we can just create a 256 byte buffer on the stack, and no serious /etc/os-release file will have lines any longer, and even if we can skip the rest. We can use getline(), I just think it's overkill.

sshedi · 2023-10-10T09:36:55Z

client/config.c

+static uint32_t TDNFParseOSInfo(PTDNF_CONF pConf)
+{
+    char temp[USERAGENTHEADERMAXLENGTH];
+    char name[10];


Dynamically allocate memory for name and version and simply assign them to pConf->pszOSName and version.
Later it gets freed anyway.

oliverkurth · 2023-10-10T15:36:33Z

client/config.c

@@ -13,6 +13,9 @@
 #include "../llconf/entry.h"
 #include "../llconf/ini.h"

+#define USERAGENTHEADERMAXLENGTH 50


Please make this better readable, use like USERAGENT_HEADER_MAXLENGTH

oliverkurth · 2023-10-10T17:14:23Z

client/config.c

+        BAIL_ON_TDNF_SYSTEM_ERROR(dwError);
+    }
+
+    dwError = TDNFAllocateMemory(10, sizeof(char), (void**)&name);


Don't pre-allocate, parse the strings into the temp buffer first, then use TDNFAllocateString() to allocate the exact amount you need.

oliverkurth · 2023-10-10T17:51:56Z

client/config.c

+    if (pConf->pszOSVersion == NULL)
+        TDNFAllocateString("UNKNOWN", &pConf->pszOSVersion);
+
+    dwError = TDNFAllocateStringPrintf(&pConf->pszUserAgentHeader, "User-Agent:tdnf/%s %s/%s", pszTdnfVersion, pConf->pszOSName, pConf->pszOSVersion);


You need a space after "User-Agent:". But, I think it doesn't need to be part of the string at all. See example at https://curl.se/libcurl/c/CURLOPT_USERAGENT.html

curl_easy_setopt(curl, CURLOPT_USERAGENT, "Dark Secret Ninja/1.0");

Also make sure it's compliant with RFC 7231, see https://www.rfc-editor.org/rfc/rfc7231#page-46

Also, I don't see anywhere where you are setting the header for curl. At some point you will have to call curl_easy_setopt(curl, CURLOPT_USERAGENT, ...

client/config.c

sshedi · 2023-10-16T06:27:55Z

client/config.c

+    file = fopen(OS_CONF_FILE, "r");
+
+    if (!file) {
+        dwError = errno;


print a warning here and return 0 perhaps?

sshedi · 2023-10-16T06:28:41Z

client/config.c

@@ -26,6 +29,50 @@ TDNFConfGetRpmVerbosity(
    return nLogLevel;
 }

+static uint32_t TDNFParseOSInfo(PTDNF_CONF pConf)
+{
+    char temp[USERAGENT_HEADER_MAX_LENGTH];


I would still say we should avoid static allocation here and use getline. It's upto you on what you want to use.

Also, be aware that fgets doesn't always null terminate the buffer. By chance if there is a line which is more than 50 bytes in os-release file, this might end up crashing tdnf.

That's not what the man page says:

fgets() reads in at most one less than size characters from stream and stores them into the buffer pointed to by s. Reading stops after an EOF or a newline. If a newline is read, it is stored into the buffer. A terminating null byte ('\0') is stored after the last character in the buffer.

Instead it seems to take care that it is aways terminated with 0.

client/config.c

sshedi · 2023-10-16T06:34:08Z

client/config.c

@@ -91,6 +139,11 @@ TDNFReadConfig(
        }
    }

+    dwError = TDNFParseOSInfo(pConf);
+    if (dwError != ENOENT) {


I have a question. If we the os-release file doesn't exist and it's considered not a fatal error, why should we consider other errors in TDNFParseOSInfo as fatal? why not use some default values in case of failure and make the return type void?

I think /etc/os-release not present is an acceptable condition, but failing to parse it for any reason is not. It may be some deeper issue (a bug in the parser, file corrupted or whatever) , and we shouldn't proceed.

sshedi · 2023-10-17T10:01:44Z

client/config.c

+    TDNF_SAFE_FREE_MEMORY(name);
+    TDNF_SAFE_FREE_MEMORY(version);
+    goto cleanup;
+


delete the empty line please

client/config.c

oliverkurth · 2023-10-17T18:16:59Z

client/config.c

+        if (strncmp("ID=", buf, sizeof("ID=")-1) == 0)
+        {
+            sscanf(buf, "ID=%s\n", buf);


You need to take care of quotes, which may or may not be present. For example this is in Ubuntu:

$ cat /etc/os-release PRETTY_NAME="Ubuntu 22.04.3 LTS" NAME="Ubuntu" VERSION_ID="22.04" VERSION="22.04.3 LTS (Jammy Jellyfish)" VERSION_CODENAME=jammy ID=ubuntu ...

This is in Photon:

# cat /etc/os-release NAME="VMware Photon OS" VERSION="5.0" ID=photon VERSION_ID=5.0 ...

Note that Ubuntu has VERSION_ID in quotes, but Photon does not. You need to check for quotes and remove them if there.

oliverkurth · 2023-10-20T19:46:47Z

pytests/tests/test_useragent.py

Thanks for writing a test! Are you going to finish this? I don't see where we are testing the header sent by tdnf (we are not calling it at all here).

oliverkurth

As stated per DM, let's drop the test change, it takes too much effort. I verified locally that the header is sent on requests. The change is good, I can merge this, without a test.

client/config.c

shivania2 · 2023-11-02T05:09:59Z

As stated per DM, let's drop the test change, it takes too much effort. I verified locally that the header is sent on requests. The change is good, I can merge this, without a test.

Thank You. I will remove the test case

oliverkurth requested changes Oct 9, 2023

View reviewed changes

sshedi reviewed Oct 10, 2023

View reviewed changes

shivania2 force-pushed the topic/shivania2/useragent branch from 46bc356 to 1293042 Compare October 10, 2023 08:25

sshedi reviewed Oct 10, 2023

View reviewed changes

shivania2 force-pushed the topic/shivania2/useragent branch from 1293042 to 8238803 Compare October 10, 2023 10:31

oliverkurth reviewed Oct 10, 2023

View reviewed changes

oliverkurth requested changes Oct 10, 2023

View reviewed changes

oliverkurth reviewed Oct 10, 2023

View reviewed changes

oliverkurth reviewed Oct 11, 2023

View reviewed changes

client/config.c Show resolved Hide resolved

shivania2 force-pushed the topic/shivania2/useragent branch from 8238803 to 12d3d44 Compare October 16, 2023 06:21

sshedi reviewed Oct 16, 2023

View reviewed changes

shivania2 force-pushed the topic/shivania2/useragent branch 4 times, most recently from a9414c2 to 74df3aa Compare October 17, 2023 06:58

shivania2 requested review from oliverkurth and sshedi October 17, 2023 08:06

sshedi reviewed Oct 17, 2023

View reviewed changes

oliverkurth requested changes Oct 17, 2023

View reviewed changes

shivania2 force-pushed the topic/shivania2/useragent branch 4 times, most recently from e547037 to d6c7975 Compare October 20, 2023 07:08

sshedi approved these changes Oct 20, 2023

View reviewed changes

oliverkurth reviewed Oct 20, 2023

View reviewed changes

shivania2 force-pushed the topic/shivania2/useragent branch 2 times, most recently from 7a52e76 to ed0a329 Compare October 26, 2023 09:42

oliverkurth reviewed Nov 1, 2023

View reviewed changes

client/config.c Show resolved Hide resolved

Set a user agent for http/https tdnf requests

95c1054

shivania2 force-pushed the topic/shivania2/useragent branch from ed0a329 to 95c1054 Compare November 2, 2023 05:10

oliverkurth merged commit ee028e5 into vmware:dev Nov 2, 2023
3 checks passed

oliverkurth mentioned this pull request Mar 2, 2024

tdnf does not set a user agent when doing http requests #451

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set a user agent for http/https tdnf requests #453

Set a user agent for http/https tdnf requests #453

shivania2 commented Oct 9, 2023

vmwclabot commented Oct 9, 2023

oliverkurth left a comment

oliverkurth Oct 9, 2023

oliverkurth Oct 9, 2023

sshedi Oct 10, 2023

oliverkurth Oct 10, 2023

oliverkurth Oct 9, 2023

sshedi Oct 10, 2023

sshedi Oct 10, 2023

oliverkurth Oct 10, 2023

sshedi Oct 10, 2023

sshedi Oct 10, 2023

sshedi Oct 10, 2023

sshedi Oct 10, 2023

shivania2 commented Oct 10, 2023

sshedi Oct 10, 2023

sshedi Oct 10, 2023

oliverkurth Oct 10, 2023

sshedi Oct 11, 2023

oliverkurth Oct 11, 2023

sshedi Oct 10, 2023

oliverkurth Oct 10, 2023

oliverkurth Oct 10, 2023

oliverkurth Oct 10, 2023 •

edited

Loading

sshedi Oct 16, 2023

sshedi Oct 16, 2023

oliverkurth Oct 17, 2023 •

edited

Loading

sshedi Oct 16, 2023

oliverkurth Oct 17, 2023

sshedi Oct 17, 2023

oliverkurth Oct 17, 2023

oliverkurth Oct 20, 2023

oliverkurth left a comment

shivania2 commented Nov 2, 2023

		uint32_t
		TDNFParseOSInfo(

		uint32_t
		TDNFParseOSInfo(

Set a user agent for http/https tdnf requests #453

Set a user agent for http/https tdnf requests #453

Conversation

shivania2 commented Oct 9, 2023

vmwclabot commented Oct 9, 2023

oliverkurth 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivania2 commented Oct 10, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverkurth Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverkurth Oct 17, 2023 • 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

oliverkurth left a comment

Choose a reason for hiding this comment

shivania2 commented Nov 2, 2023

oliverkurth Oct 10, 2023 •

edited

Loading

oliverkurth Oct 17, 2023 •

edited

Loading