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

ipa-join: implement calls to JSON-RPC endpoints #4724

Closed
wants to merge 10 commits into from

Conversation

Carbenium
Copy link
Contributor

Based on work done by @mulatinho in PR #3544

@abbra
Copy link
Contributor

abbra commented Jun 1, 2020

autoheader: warning: missing template: WITH_IPA_JOIN_XML
autoheader: Use AC_DEFINE([WITH_IPA_JOIN_XML], [], [Description])
autoreconf: /usr/bin/autoheader failed with exit status: 1
configure: error: cannot find install-sh, install.sh, or shtool in "." "./.." "./../.."

@Carbenium Carbenium force-pushed the ipa-join-json-rpc branch 5 times, most recently from 0c60b59 to 544d156 Compare June 1, 2020 13:09
freeipa.spec.in Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
client/ipa-join.c Show resolved Hide resolved
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

Excellent work! I'm impressed.

I have marked a couple of places where NULL or error checks are missing. Explicit error checking in C is a PITB. :(

Normally I wouldn't bee too concerned with memory leaks in short-running binaries. But our internal build systems and QA has static code analyzers that will flag any potential leak.

client/ipa-join.c Outdated Show resolved Hide resolved
client/ipa-join.c Outdated Show resolved Hide resolved
client/ipa-join.c Outdated Show resolved Hide resolved
client/ipa-join.c Outdated Show resolved Hide resolved
client/ipa-join.c Outdated Show resolved Hide resolved
client/ipa-join.c Outdated Show resolved Hide resolved
client/ipa-join.c Show resolved Hide resolved
client/ipa-join.c Outdated Show resolved Hide resolved
client/ipa-join.c Outdated Show resolved Hide resolved
client/ipa-join.c Outdated Show resolved Hide resolved
@Carbenium Carbenium force-pushed the ipa-join-json-rpc branch 2 times, most recently from caf6f4e to 8f919a8 Compare June 2, 2020 23:59
@Carbenium
Copy link
Contributor Author

@rcritten @tiran Thanks for the reviews so far! It's been a while I had been in contact with C...

I think/hope I addressed all your comments - if you want to have another go.
JSON-RPC based unenrollment is now implemented as well, so xmlrpc is completely optional.

@Carbenium Carbenium changed the title ipa-join: use JSON-RPC endpoints for enrollment ipa-join: implement calls to JSON-RPC endpoints Jun 3, 2020
client/ipa-join.c Outdated Show resolved Hide resolved
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

clang code analyzer doesn't four lines of code:

$ scan-build make
...
ipa-join.c:690:34: warning: Result of 'malloc' is converted to a pointer of type 'char', which is incompatible with sizeof operand type 'char *'
    response->payload = (char *) malloc(sizeof(response->payload));
                         ~~~~~~  ^~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~
ipa-join.c:941:9: warning: Branch condition evaluates to a garbage value
    if (cb.payload)
        ^~~~~~~~~~
ipa-join.c:1026:9: warning: Branch condition evaluates to a garbage value
    if (cb.payload)
        ^~~~~~~~~~
ipa-join.c:1400:9: warning: Value stored to 'krberr' is never read
        krberr = krb5_cc_initialize(krbctx, ccache, creds.client);
        ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4 warnings generated.

@Carbenium
Copy link
Contributor Author

@tiran Issues found by clang are fixed. I'm going to move the fixes to the respective commits once you reviewed them.

@abbra abbra added the re-run Trigger a new run of PR-CI label Jun 7, 2020
client/ipa-join.c Outdated Show resolved Hide resolved
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jun 7, 2020
@Carbenium Carbenium force-pushed the ipa-join-json-rpc branch 2 times, most recently from 36efe85 to 0d27eed Compare June 7, 2020 10:54
@abbra abbra added the re-run Trigger a new run of PR-CI label Jun 8, 2020
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jun 8, 2020
@abbra
Copy link
Contributor

abbra commented Jun 8, 2020

checking for JANSSON... yes
checking for LIBCURL... no
configure: error: Package requirements (libcurl) were not met:
Package 'libcurl', required by 'virtual:world', not found
Consider adjusting the PKG_CONFIG_PATH environment variable if you
installed software in a non-standard prefix.
Alternatively, you may set the environment variables LIBCURL_CFLAGS
and LIBCURL_LIBS to avoid the need to call pkg-config.
See the pkg-config man page for more details.
error: Bad exit status from /var/tmp/rpm-tmp.wJxFjt (%build)
RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.wJxFjt (%build)
Child return code was: 1

@abbra abbra added ipa-next Mark as master (4.12) only re-run Trigger a new run of PR-CI labels Jun 8, 2020
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jun 8, 2020
@abbra abbra added the re-run Trigger a new run of PR-CI label Jun 8, 2020
@rcritten
Copy link
Contributor

rcritten commented Jul 6, 2020

@Carbenium Can you rebase this against the current master? CI is failing because some code that controls template versions is out-of-date.

@rcritten rcritten added the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Jul 6, 2020
mulatinho and others added 10 commits July 6, 2020 22:21
Adding JSON-C and LibCURL library into configure.ac and Makefile.am

Creating a API call with option '-j' or '--jsonrpc' to make host join
on FreeIPA with JSONRPC and libCURL.

Related: https://pagure.io/freeipa/issue/7966
Signed-off-by: Alexandre Mulatinho <alex@mulatinho.net>
CURLOPT_WRITEFUNCTION is not guaranteed to be called only
once per request and receive all data at once.
Use a dynamic buffer to cope with that case.

Related: https://pagure.io/freeipa/issue/7966
Additionally JSON-RPC should bail out if host is already joined.
Check HTTP status of JSON-RPC request and report 401 Unauthorized error explicitly.

Related: https://pagure.io/freeipa/issue/7966
The used RPC protocol (JSON or XML) is defined
at build time.

Related: https://pagure.io/freeipa/issue/7966
@Carbenium
Copy link
Contributor Author

@rcritten Sure thing. Here you go.

@fcami fcami added the re-run Trigger a new run of PR-CI label Jul 6, 2020
@fcami
Copy link
Contributor

fcami commented Jul 6, 2020

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fcami
Copy link
Contributor

fcami commented Jul 6, 2020

Triggering Azure CI + PR-CI as the Azure failure is unrelated to the PR.

@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 6, 2020
@fcami
Copy link
Contributor

fcami commented Jul 7, 2020

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rcritten
Copy link
Contributor

rcritten commented Jul 7, 2020

All tests are passing, this looks good to me. Last chance @abbra @tiran any final comments before I ack?

@rcritten rcritten removed the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Jul 7, 2020
@abbra
Copy link
Contributor

abbra commented Jul 9, 2020

LGTM.

So this is going to be a change in defaults: we would default to using JSON-RPC now unless configure --with-ipa-join-xml would be done explicitly.

I think it is worth to document in the release notes (add 'changelog' entry in the ticket') because this client would not be able to enroll to very old servers (IPA v2.x?). No practical change to anything else, though.

@abbra abbra added the ack Pull Request approved, can be merged label Jul 9, 2020
@abbra
Copy link
Contributor

abbra commented Jul 9, 2020

master:

  • 6e414d2 ipa-join: allowing call with jsonrpc into freeipa API
  • 5e7e4f0 ipa-join: don't set TLS related curl options for JSON-RPC
  • c197918 ipa-join: improve curl error handling in JSON-RPC code
  • c905f94 ipa-join: buffer curl response before parsing json
  • 25205f4 ipa-join: switch to jansson for json handling
  • 677659c ipa-join: extract unenrollment code common to JSON and XML-RPC to separate function
  • 62503e4 ipa-join: implement JSON-RPC based unenrollment
  • f694077 ipa-join: select {JSON,XML}-RPC at build time
  • a1b117a ipa-join: Use bool type where appropriate
  • 7cc977b ipa-join: Generalize XML-RPC references in man page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged ipa-next Mark as master (4.12) only pushed Pull Request has already been pushed
Projects
None yet
7 participants