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

Fix the empty host column from logged_in_users table #7685

Merged
merged 5 commits into from
Nov 9, 2022

Conversation

sm0k
Copy link
Contributor

@sm0k sm0k commented Jul 12, 2022

Fixes empty "host" column from logged_in_users table

@sm0k sm0k requested review from a team as code owners July 12, 2022 08:27
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 12, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@mike-myers-tob mike-myers-tob added virtual tables ready for review Pull requests that are ready to be reviewed by a maintainer Windows and removed ready for review Pull requests that are ready to be reviewed by a maintainer labels Jul 12, 2022
Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

Did you run into trouble with the CLA signing?

osquery/tables/system/windows/logged_in_users.cpp Outdated Show resolved Hide resolved
@sm0k
Copy link
Contributor Author

sm0k commented Jul 17, 2022

Did you run into trouble with the CLA signing?

Didn't think this was mandatory, will do tomorrow

@Smjert
Copy link
Member

Smjert commented Jul 17, 2022

Are we sure that the empty case it's not an error caused by this?

auto addr = reinterpret_cast<const char*>(wtsClient->ClientAddress);
r["host"] = std::string(addr, CLIENTADDRESS_LENGTH);

Because as I far as I can understand we are reinterpreting the IPv6 bytes as characters, not converting those bytes to digits that can be printed as with IPv4 case.
So maybe one of the initial bytes could be 0 and that gets printed/serialized as an empty string.

How were you able to reproduce this issue?

@sm0k
Copy link
Contributor Author

sm0k commented Jul 17, 2022

Are we sure that the empty case it's not an error caused by this?

auto addr = reinterpret_cast<const char*>(wtsClient->ClientAddress);
r["host"] = std::string(addr, CLIENTADDRESS_LENGTH);

Because as I far as I can understand we are reinterpreting the IPv6 bytes as characters, not converting those bytes to digits that can be printed as with IPv4 case. So maybe one of the initial bytes could be 0 and that gets printed/serialized as an empty string.

How were you able to reproduce this issue?

Here is my debug session, with a RDP connected user:
Capture d’écran du 2022-07-17 23-33-09

Looks like WTSQuerySessionInformationW using the WTSClientInfo _WTS_INFO_CLASS is not following the expected behavior described in MSDN, reason why I chose to use WTSClientName in case of an empty host. Maybe its a Microsoft related bug?

@sm0k
Copy link
Contributor Author

sm0k commented Jul 18, 2022

Did you run into trouble with the CLA signing?

I signed the CLA yesterday, is there anything more to do?

@Smjert
Copy link
Member

Smjert commented Jul 18, 2022

/easycla

@Smjert
Copy link
Member

Smjert commented Jul 18, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@sm0k
Copy link
Contributor Author

sm0k commented Jul 18, 2022

Did you run into trouble with the CLA signing?

I signed the CLA yesterday, is there anything more to do?

@sm0k There seems to be a problem with the commit itself which is missing the User ID. As far as I recall this means that you have created and/or pushed that commit with a user and mail that's not the one you have here on Github, so EasyCLA is unable to connect the commit with your Github account

Hm that's strange, the original commit was done using my ssh pub key, linked to this account... How can I fix this? Sorry, not really familiar with easyCLA here.

@Smjert
Copy link
Member

Smjert commented Jul 18, 2022

Hm that's strange, the original commit was done using my ssh pub key, linked to this account... How can I fix this? Sorry, not really familiar with easyCLA here.

It's more a problem of the author/email in the commit itself. Is the email in the commit associated with the Github account?
The email you have there has to be one of the primary or secondary emails you have in the account.

@sm0k
Copy link
Contributor Author

sm0k commented Jul 18, 2022

Hm that's strange, the original commit was done using my ssh pub key, linked to this account... How can I fix this? Sorry, not really familiar with easyCLA here.

It's more a problem of the author/email in the commit itself. Is the email in the commit associated with the Github account? The email you have there has to be one of the primary or secondary emails you have in the account.

Ok, got it, the commit was done using my professional email. Do you want me to close this PR and open a clean one?

@Smjert
Copy link
Member

Smjert commented Jul 18, 2022

Ok, got it, the commit was done using my professional email. Do you want me to close this PR and open a clean one?

It's not necessary, either modify the commit author with git commit --amend --author="..." --no-edit or set for the osquery repository your user and email with git config user.name <youruser> and git config user.email <your email>, then git commit --amend --reset-author --no-edit.
The second method would be permanent for that folder/repository, so that if you want to contribute again you don't have to do this anymore.

Finally push force the branch, and the PR will update.

One unrelated thing for the future: I would suggest to use a dedicated branch in your repository for the PR to be pointed at instead of master.

@sm0k
Copy link
Contributor Author

sm0k commented Jul 18, 2022

Thank you, should be good now

@Smjert
Copy link
Member

Smjert commented Jul 20, 2022

I'm not sure what happened but I had to restore a couple of our comments because the EasyCLA bot decided to edit them and put there the update on its status.

image

@sm0k
Copy link
Contributor Author

sm0k commented Jul 25, 2022

Ok thank you, are you waiting fro an action from me to close the change request?

Copy link
Member

@Smjert Smjert left a comment

Choose a reason for hiding this comment

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

Hello @sm0k, sorry for waiting.
The code works, but I'd like to have a final small change.

osquery/tables/system/windows/logged_in_users.cpp Outdated Show resolved Hide resolved
@mike-myers-tob
Copy link
Member

Hello @sm0k, sorry for waiting. The code works, but I'd like to have a final small change.

Did the final small change address all the feedback? Can this PR be approved?

@mike-myers-tob mike-myers-tob changed the title Fixes empty "host" column from logged_in_users table Fix the empty host column from logged_in_users table Aug 29, 2022
@mike-myers-tob mike-myers-tob merged commit f8bd96e into osquery:master Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants