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

proposal: x/sys/windows: add various bcrypt.dll, crypt32.dll and ncrypt.dll functions #64819

Open
peter-v42 opened this issue Dec 20, 2023 · 9 comments
Labels
Milestone

Comments

@peter-v42
Copy link

Proposal Details

For an application I needed a number of functions to sign data with a PKI certificate stored on a smartcard (so the private key is not accessible). I would like to contribute these functions to x/sys/windows so I do not have to implement them in my own code. I used the existing x/sys/windows code as a reference, so my code should fit in nicely. The only question I still have is whether I'm allowed to use the new syscall.SyscallN() function: as far as I saw it's not yet being used in x/sys/windows right now (but I did not search all files for it).

@gopherbot gopherbot added this to the Proposal milestone Dec 20, 2023
@dagood
Copy link
Contributor

dagood commented Dec 21, 2023

The only question I still have is whether I'm allowed to use the new syscall.SyscallN() function

For that issue, what ended up getting merged is a fix to use SyscallN if the number of params is > 15 (and <= 42). Even though the numbered SyscallX functions are deprecated, continuing to use them avoids unnecessary code churn.

After that issue was fixed, I didn't add any of the new >15 param syscalls to x/sys/windows directly, I just wanted the tool to support this when it's used in new places and third party modules. (Here's an example of where I'm using it: https://github.com/microsoft/go-crypto-winnative/blob/main/internal/bcrypt/bcrypt_windows.go.)

@peter-v42
Copy link
Author

@dagood , thanks for the explanation. But why put those bcrypt functions into a separate module instead of in x/sys/windows? A number of these functions are also required by my application and that is exactly the reason why I want to have them in x/sys/windows instead of in my own code.

One more thought: should I add 20 new functions in x/sys/windows using one proposal. or would it be better to split this into multiple separate proposals? What I have is 7 bcrypt.dll functions, 7 crypt32.dll functions, 2 user32.dll functions, 2 kernel32.dll functions, 1 advapi32.dll function and 1 ncrypt.dll function.

@dagood
Copy link
Contributor

dagood commented Jan 3, 2024

But why put those bcrypt functions into a separate module instead of in x/sys/windows?

In microsoft/go-crypto-winnative it's useful to be able to add/change syscall implementations without involving upstream and new versions of x/sys. For another example, https://github.com/microsoft/go-winio also has a lot of syscall implementations. But these examples just show one way to get unblocked, and as far as I know, it's fine to contribute the new syscalls to x/sys/windows.

I see that some recent new syscalls were tracked as bugs rather than proposals, so the new ones might be easy to accept:

I'd go ahead and provide the list of syscalls for now if you can--I'm not familiar with the bar for accepting new ones, and I think others can help split up the list if necessary.

(CC @golang/windows)

@alexbrainman
Copy link
Member

One more thought: should I add 20 new functions in x/sys/windows using one proposal. or would it be better to split this into multiple separate proposals? What I have is 7 bcrypt.dll functions, 7 crypt32.dll functions, 2 user32.dll functions, 2 kernel32.dll functions, 1 advapi32.dll function and 1 ncrypt.dll function.

@peter-v42

As far as I know, you don't need a proposal to add Windows APIs. It is fine to just send a CL with your changes.

The only problem is for someone to review its correctness, because we won't be able change them once merged. But maybe we can all review them together.

Alternatively you can just use https://github.com/microsoft/go-crypto-winnative/blob/main/internal/bcrypt/bcrypt_windows.go as suggested by @dagood . Or even have your own version. Or maybe contribute whatever you need to https://github.com/microsoft/go-crypto-winnative/blob/main/internal/bcrypt/bcrypt_windows.go if Microsoft are OK with your contribution.

Thank you.

Alex

@peter-v42
Copy link
Author

peter-v42 commented Jan 23, 2024

As this is going to be the first time contributing to the go code, I do prefer to do it the go way. But the learning curve seems to be a bit steep. I've read the contribution guide and the referenced documents, but I still don't dare to start. Is there a more detailed step-by-step guide somewhere about contributing to the x/sys package?

  • Is there a guideline for the scope of a proposal (or CL)? Can I add 20 new API calls in syscall_windows.go in one proposal or CL?
  • Who determines the name of the git branch I'm going to use to contribute my changes?
  • Using go generate is new to me, but it seems to work flawlessly. Anything I should worry about?
  • Did somebody already compile a translation table from windows types to go types? I've create my own from the existing API calls ...

For starters I'll just start working and testing with a branch in a local copy. Once I've implemented and tested all the API calls I need, I'll get back here to try to find the correct (go) way to contribute my additions.

One more note: it looks like x/sys is the chosen go way to implement low-level system specific functionality, so I prefer to do it the go way instead of creating or adding to a parallel effort to implement the same functionality in other packages.

@dagood
Copy link
Contributor

dagood commented Jan 24, 2024

I'm not aware of a more specific guide, personally I was able to get started with https://go.dev/doc/contribute. It includes a brief step-by-step at https://go.dev/doc/contribute#gerrit_overview, but then also has a much more in depth step-by-step starting at https://go.dev/doc/contribute#checkout_go. They have some steps that are specific to the golang.org/x/... repos.

Submitting via GitHub rather than Gerrit is also valid, and it might be easier to find a detailed guide for it because GitHub's more widely used.

For the bulleted questions, I'll take a shot (as a fellow contributor, to be clear):

  • Is there a guideline for the scope of a proposal (or CL)? Can I add 20 new API calls in syscall_windows.go in one proposal or CL?

In general, for non-trivial contributions you can just file an issue first to figure this out. Providing the list of API calls in this issue might let someone proactively point out any that might deserve more specific attention, but I doubt that will be a problem here and going ahead with the CL would be fine.

  • Who determines the name of the git branch I'm going to use to contribute my changes?

The branch name doesn't actually affect the CL submission process on Gerrit. It's only for your use, to e.g. organize multiple CLs locally.

On GitHub, the branch name shows up in the pull request UI, but it also isn't important.

  • Using go generate is new to me, but it seems to work flawlessly. Anything I should worry about?

I don't think so, in this case. Perhaps keep in mind that go generate can run arbitrary code, so don't use it in a project you don't trust. I'm not aware of any gotchas with x/sys in particular here.

  • Did somebody already compile a translation table from windows types to go types? I've create my own from the existing API calls ...

I'm not aware of one. However, there is not only one way to translate a given parameter. For example, both outBuffer *byte, outBufferLen uint32 and pbOutput []byte are valid.

I don't remember seeing a place where the rules/possibilities are laid out. I hope I just missed it, that would be nice to see!

@peter-v42
Copy link
Author

@dagood thanks for your extensive reply. I'll document what I'm doing, step-by-step, as well as the parameter translations.
This might be helpful to future contributors so I'll make sure to publish it somewhere once I'm done.

@dtangster
Copy link

As part of this work, can we also introduce SetFilePointerEx? I'm not sure why, but the original ticket #61834 was resolved but this particular function wasn't added.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/572255 mentions this issue: x/sys/windows: add various bcrypt.dllfunctions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

5 participants